Skip to content

[@types/prop-types] Add support of as const to oneOf#34706

Merged
sandersn merged 2 commits intoDefinitelyTyped:masterfrom
ibezkrovnyi:prop-types-fix-oneof
Apr 17, 2019
Merged

[@types/prop-types] Add support of as const to oneOf#34706
sandersn merged 2 commits intoDefinitelyTyped:masterfrom
ibezkrovnyi:prop-types-fix-oneof

Conversation

@ibezkrovnyi
Copy link
Copy Markdown
Contributor

@ibezkrovnyi ibezkrovnyi commented Apr 12, 2019 •
edited
Loading

Common way before microsoft/TypeScript#29510 was to use tuple trick

export const tuple = <T extends string[]>(...args: T) => args;
const sizes = tuple('default', 'large');
export type SizeType = typeof sizes[number];

export const Button = (props: { size: SizeType }) => <div></div>;

Button.propTypes = {
  size: PropTypes.oneOf(sizes),
};

After microsoft/TypeScript#29510 has landed we could use as const

const sizes = ['default', 'large'] as const;
export type SizeType = typeof sizes[number];

export const Button = (props: { size: SizeType }) => <div></div>;

Button.propTypes = {
  size: PropTypes.oneOf(sizes),   // Error: Type 'readonly ["default", "large"]' is missing the following properties from type '{}[]': pop, push, reverse, shift and 6 more. [2345]
};

Some evidence

tuple is pretty common approach, for example, it is used in ant.design

Change

Change is to replace type T[] of oneOf parameter with ReadonlyArray<T>.
This change relaxes requirement for an Array.

Also I tried readonly T[] and it works fine too. But this is pretty new feature

Tested

This change was tested on my project and I used as const in part of code location, while keeping old way for another part of code locations. Both old and new ways work fine.

@ibezkrovnyi
Copy link
Copy Markdown
Contributor Author

ibezkrovnyi commented Apr 12, 2019 •
edited
Loading

Build Failed.
Some issue with typescript@3.5.0-dev20190412 (next) and recompose/recompose-test.tsx
Reverting my changes didn't help. It is probably some change introduced by new version of TypeScript compiler. After moving to TypeScript@3.4.3 issues has gone.

image

image

image

@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). The Travis CI build failed labels Apr 12, 2019
@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Apr 12, 2019 •
edited
Loading

@ibezkrovnyi Thank you for submitting this PR!

🔔 @DovydasNavickas @ferdaber @eps1lon - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Apr 12, 2019 •
edited
Loading

@ibezkrovnyi The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@ibezkrovnyi
Copy link
Copy Markdown
Contributor Author

ibezkrovnyi commented Apr 13, 2019 •
edited
Loading

Issue

Issue is in enhancer4 of /types/recompose/recompose-tests.tsx
Some screenshots: #34706 (comment)
It looks like type inference an some point is broken now andunknown type is inferred.

Tested

After reverting my changes (using master branch):

typescript@3.4.3: ✔️ no issue
typescript@3.5.0-dev.20190404: ✔️ no issue
typescript@3.5.0-dev.20190409: ✔️ no issue
typescript@3.5.0-dev.20190411: ✔️ no issue
typescript@3.5.0-dev.20190412: ✖️ issue in recompose
typescript@3.5.0-dev.20190413: ✖️ issue in recompose
typescript@3.5.0-dev.20190416: ✖️ issue in recompose
typescript@3.5.0-dev.20190417: ✔️ no issue !

@RyanCavanaugh could you please advise what should I do - while issue with recompose isn't related to this PR, Travis CI build of this PR failed

@ferdaber
Copy link
Copy Markdown
Contributor

ferdaber commented Apr 15, 2019

ReadonlyArray<T> is already a subtype of Array<T>, so wouldn't this change make the parameter more restrictive for those who just have regular arrays?

@ibezkrovnyi
Copy link
Copy Markdown
Contributor Author

ibezkrovnyi commented Apr 15, 2019

@ferdaber

  1. ReadonlyArray<T> is not subtype of Array<T>, it has no push, pop, shift, etc. Inside lib.es5.d.ts Array and ReadonlyArray are two separate interfaces.

  2. ReadonlyArray is less restrictive, as it doesn't require from argument to have methods push, pop, etc.

  3. Changed oneOf was tested with both ReadonlyArrays and with Arrays too and it works fine with both

Tested

This change was tested on my project and I used as const in part of code location, while keeping old way for another part of code locations. Both old and new ways work fine.

@ferdaber
Copy link
Copy Markdown
Contributor

ferdaber commented Apr 15, 2019

Derp, I went the opposite way. Sounds good, thanks for the clarification!

@typescript-bot typescript-bot added the Owner Approved A listed owner of this package signed off on the pull request. label Apr 15, 2019
@ibezkrovnyi
Copy link
Copy Markdown
Contributor Author

ibezkrovnyi commented Apr 17, 2019

Removed non-consistent periods in JSDoc to trigger CI build

@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Apr 17, 2019

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

@sandersn sandersn merged commit 3ea6564 into DefinitelyTyped:master Apr 17, 2019
@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Apr 17, 2019

I just published @types/prop-types@15.7.1 to npm.

@ibezkrovnyi ibezkrovnyi deleted the prop-types-fix-oneof branch April 18, 2019 17:05
alesn pushed a commit to alesn/DefinitelyTyped that referenced this pull request Apr 23, 2019
@ibezkrovnyi @alesn
[@types/prop-types] Add support of as const to oneOf (DefinitelyTyp…
0ce5ff3 
…ed#34706)

* Add readonly modifier to oneOf definition

* remove unnecessary periods, trigger ci build
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@ferdaber ferdaber ferdaber approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Owner Approved A listed owner of this package signed off on the pull request. Popular package This PR affects a popular package (as counted by NPM download counts).

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ibezkrovnyi @typescript-bot @ferdaber @sandersn