Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bump typescript package to latest version #1538

Merged
merged 4 commits into from
Dec 2, 2021
Merged

bump typescript package to latest version #1538

merged 4 commits into from
Dec 2, 2021

Conversation

shiftkey
Copy link
Contributor

I've found a bug in the generated typings that seems to be tied to being on an old version of Typescript.

To reproduce it, run this command after building the package:

$ ./node_modules/.bin/tsc lib/TextInputWithTokens.d.ts

Without this fix, one of the errors you'll see is this:

lib/TextInputWithTokens.d.ts:43:4 - error TS2344: Type 'string | number | symbol' does not satisfy the constraint 'string | number'.
  Type 'symbol' is not assignable to type 'string | number'.

43 }, string | number | symbol>, "maxWidth" | "minWidth" | "width" | "theme" | "className" | "block" | "icon" | "sx" | "disabled" | "variant" | "contrast"> & {
      ~~~~~~~~~~~~~~~~~~~~~~~~

I can reproduce this with three files under lib after building:

$ ./node_modules/.bin/tsc lib/Autocomplete/AutocompleteInput.d.ts
$ ./node_modules/.bin/tsc lib/SelectMenu/SelectMenu.d.ts
$ ./node_modules/.bin/tsc lib/TextInputWithTokens.d.ts

I think this is just a fix for how Pick<T, Key> gets generated, as it seems like the generated types for these files all have this in common:

Pick<{
    [x: string]: any;
    [x: number]: any;
} & {
    theme?: any;
} & {
    as?: string | React.ComponentType<any> | undefined;
    forwardedAs?: string | React.ComponentType<any> | undefined;
}, string | number | symbol> // note the "symbol" mentioned here is not a part of the `Pick` above.

With this version, you'll now see this as part of the signature:

Pick<{
    [x: string]: any;
    [x: number]: any;
    [x: symbol]: any;        // this new line now gets the type in sync with the keys below
} & {
    theme?: any;
} & {
    as?: string | React.ComponentType<any> | undefined;
    forwardedAs?: string | React.ComponentType<any> | undefined;
}, string | number | symbol>

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@shiftkey shiftkey requested review from a team and rezrah October 25, 2021 20:28
@changeset-bot
Copy link

changeset-bot bot commented Oct 25, 2021

🦋 Changeset detected

Latest commit: 6235cf4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/components Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@shiftkey shiftkey added the dependencies Pull requests that update a dependency file label Oct 25, 2021
Copy link
Contributor

@rezrah rezrah left a comment

Choose a reason for hiding this comment

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

@shiftkey - I've just verified the fix. Thanks for putting this in. Feel free to merge it after conflicts are resolved, or happy to do this for you if it's easier.

@shiftkey
Copy link
Contributor Author

shiftkey commented Dec 2, 2021

@rezrah I've just merged main into this branch and resolved this conflicts

@colebemis colebemis enabled auto-merge (squash) December 2, 2021 16:52
@colebemis colebemis merged commit dda6e5d into primer:main Dec 2, 2021
@primer-css primer-css mentioned this pull request Dec 2, 2021
pksjce pushed a commit that referenced this pull request Dec 20, 2021
* bump typescript package to latest version

* add changeset file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants