Skip to content
This repository has been archived by the owner on Oct 1, 2024. It is now read-only.

use typescript native types #2163

Merged
merged 1 commit into from
Feb 10, 2022
Merged

Conversation

m4thieulavoie
Copy link
Contributor

Description

This PR is a first of two steps to get rid of Omit, Arguments, ConstructorArguments and ThenType in favour of their native Typescript counterpart.

Fixes (issue #)

Type of change

  • react-async Patch: No impact Omit usage change (non-breaking change which fixes an issue)

  • react-form-state Patch: No impact Omit usage change (non-breaking change which fixes an issue)

  • react-graphql Patch: No impact Omit usage change (non-breaking change which fixes an issue)

  • react-testing Patch: No impact Omit and Arguments usage change (non-breaking change which fixes an issue)

  • useful-types Patch: No impact ArgumentAtIndex and FirstArgument change to accept Parameters (non-breaking change which fixes an issue)

  • Patch: Bug (non-breaking change which fixes an issue)

  • Minor: New feature (non-breaking change which adds functionality)

  • Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have added a changelog entry, prefixed by the type of change noted above (Documentation fix and Test update does not need a changelog as we do not publish new version)

@m4thieulavoie m4thieulavoie requested a review from BPScott February 8, 2022 00:09
@m4thieulavoie m4thieulavoie self-assigned this Feb 8, 2022
@m4thieulavoie m4thieulavoie requested a review from a team as a code owner February 8, 2022 00:09
@m4thieulavoie m4thieulavoie force-pushed the use-native-typescript-types branch 3 times, most recently from 7889810 to 1b0df1c Compare February 8, 2022 00:46
@BPScott
Copy link
Member

BPScott commented Feb 8, 2022

As we're now no longer depending on @shopify/useful-types in several places, we might have packages that no longer depend upon @shopify/useful-types. For instance it looks like that was the only usage in react-form-state.

Can you identify these cases where a package no longer depends upon @shopify/useful-types, and then in those packages remove the dependency from package.json and the tsconfig.json.

@BPScott
Copy link
Member

BPScott commented Feb 8, 2022

MaybeFunctionReturnType looks very similar to ReturnType, I wonder if that's a meaningful difference, and thus if it's worth removing MaybeFunctionReturnType?

export type MaybeFunctionReturnType<T> = T extends (...args: any[]) => infer U
  ? U
  : never;


type ReturnType<T extends (...args: any) => any> = T extends (...args: any) => infer R ? R : any;

@m4thieulavoie
Copy link
Contributor Author

As we're now no longer depending on @shopify/useful-types in several places, we might have packages that no longer depend upon @shopify/useful-types. For instance it looks like that was the only usage in react-form-state.

Can you identify these cases where a package no longer depends upon @shopify/useful-types, and then in those packages remove the dependency from package.json and the tsconfig.json.

That's done, but it affected some unit tests. Let me know if I handled that properly!

@m4thieulavoie
Copy link
Contributor Author

MaybeFunctionReturnType looks very similar to ReturnType, I wonder if that's a meaningful difference, and thus if it's worth removing MaybeFunctionReturnType?

export type MaybeFunctionReturnType<T> = T extends (...args: any[]) => infer U
  ? U
  : never;


type ReturnType<T extends (...args: any) => any> = T extends (...args: any) => infer R ? R : any;

Tried to do it yesterday, but I guess I was tired, but browsing reddit I found exactly what I was missing 😅 that should be done!

@@ -11,5 +11,5 @@
"./src/**/*.tsx"
],
"exclude": ["**/test/**/*", "**/tests/**/*"],
"references": [{"path": "../graphql-typed"}, {"path": "../useful-types"}]
"references": [{"path": "../graphql-typed"}]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gosh I feel dumb now 🤦

@BPScott
Copy link
Member

BPScott commented Feb 8, 2022

I've pushed a commit that reverts the typescript-project-references.test.ts changes and applies the proper fix.

That test asserts that "if you use a dependency on another package in this monorepo in a package's package.json, then there should be a corresponding entry in that package's tsconfig.json reference field" (as this repo uses project references - one ts project per package)

You've removed react-server's dependency on useful-types in the package.json, but you did not remove the reference to the useful-types folder in react-server's tsconfig.json. And thus the test fails, as there should be mention of a dependant package (or in this case no mention of a package that is not depended upon) in both package.json and tsconfig.json.

The fix is not to modify the typescript-project-references.test.ts, but to update the tsconfig.json files, removing the references to useful-types in the packages that no longer depend upon useful-types

@m4thieulavoie
Copy link
Contributor Author

I've pushed a commit that reverts the typescript-project-references.test.ts changes and applies the proper fix.

That test asserts that "if you use a dependency on another package in this monorepo in a package's package.json, then there should be a corresponding entry in that package's tsconfig.json reference field" (as this repo uses project references - one ts project per package)

You've removed react-server's dependency on useful-types in the package.json, but you did not remove the reference to the useful-types folder in react-server's tsconfig.json. And thus the test fails, as there should be mention of a dependant package (or in this case no mention of a package that is not depended upon) in both package.json and tsconfig.json.

The fix is not to modify the typescript-project-references.test.ts, but to update the tsconfig.json files, removing the references to useful-types in the packages that no longer depend upon useful-types

Nice! I totally missed that packages had internal tsconfig.json, that's my bad!

Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pushed up another commit where I spotted dangling usages of FirstArgument and Arguments.

  • Packages that no longer depend upon useful-types should have an entry in their CHANGELOG.md. Remember to reference this PR, like the other changelog entries
  • I reckon we can revert the change in useful-types - we're due to delete these types once we update this PR anyway so I don't think there's much value in adjusting them now.

@@ -2,10 +2,13 @@ export type ThenType<T> = T extends Promise<infer U> ? U : T;

export type Arguments<T> = T extends (...args: infer U) => any ? U : never;
export type ArgumentAtIndex<
Func,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the plan is to remove Argument, ArgumentAtIndex and FirstArgument imminently then do we need to make this change now?

I'd be tempted to revert this, and the change to packages/useful-types/CHANGELOG.md

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, just reverted it

@m4thieulavoie m4thieulavoie force-pushed the use-native-typescript-types branch 2 times, most recently from 6308400 to 554db48 Compare February 8, 2022 19:20
@m4thieulavoie m4thieulavoie requested a review from BPScott February 8, 2022 19:21
Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes look good to me.

Can you add changelog entries to the packages that no longer depend upon useful-types

@@ -10,6 +10,7 @@ and adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
### Added

- Added `PartialSome` and `RequireSome` types to set specified fields of a property to optional or required
- Use Typescript native types and pave the ground to deprecate legacy types
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be removed, as we're no longer touching useful-types

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@m4thieulavoie m4thieulavoie force-pushed the use-native-typescript-types branch from 554db48 to d71578e Compare February 9, 2022 13:11
@m4thieulavoie m4thieulavoie requested a review from BPScott February 9, 2022 13:11
@m4thieulavoie m4thieulavoie force-pushed the use-native-typescript-types branch from 84cc512 to e9b40c9 Compare February 9, 2022 13:27
packages/performance/CHANGELOG.md Outdated Show resolved Hide resolved
packages/react-async/CHANGELOG.md Outdated Show resolved Hide resolved
packages/react-form-state/CHANGELOG.md Outdated Show resolved Hide resolved
packages/graphql-testing/CHANGELOG.md Outdated Show resolved Hide resolved
packages/react-graphql/CHANGELOG.md Outdated Show resolved Hide resolved
packages/react-html/CHANGELOG.md Outdated Show resolved Hide resolved
packages/react-network/CHANGELOG.md Outdated Show resolved Hide resolved
packages/react-server/CHANGELOG.md Outdated Show resolved Hide resolved
packages/react-testing/CHANGELOG.md Outdated Show resolved Hide resolved
@m4thieulavoie m4thieulavoie force-pushed the use-native-typescript-types branch from e9b40c9 to ab170d7 Compare February 9, 2022 21:10
packages/react-html/CHANGELOG.md Outdated Show resolved Hide resolved
packages/react-server/CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the changelog entries to use the "changed" subheading and add the link to the PR

changelog

forgot this one

more

apply pr comments

Revert typescript-project-references.test.ts and apply proper fix

remove two more usages of FirstArgument and Arguments

revert ArgumentAtIndex changes

update changelog files

fix tests

Apply suggestions from code review

Apply suggestions from code review
@m4thieulavoie m4thieulavoie force-pushed the use-native-typescript-types branch from e58e87b to 6463ec4 Compare February 9, 2022 21:13
@m4thieulavoie m4thieulavoie merged commit 33b66b2 into main Feb 10, 2022
@m4thieulavoie m4thieulavoie deleted the use-native-typescript-types branch February 10, 2022 13:20
@shopify-shipit shopify-shipit bot temporarily deployed to production February 14, 2022 16:32 Inactive
@shopify-shipit shopify-shipit bot temporarily deployed to fix-deprecated-exports-pattern February 22, 2022 19:45 Inactive
@shopify-shipit shopify-shipit bot temporarily deployed to a11y_tests_class_improvement February 24, 2022 21:31 Inactive
@shopify-shipit shopify-shipit bot temporarily deployed to mf-beta March 7, 2022 09:18 Inactive
@shopify-shipit shopify-shipit bot temporarily deployed to production-gem June 21, 2022 14:45 Inactive
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants