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

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

Merged
merged 2 commits into from
Apr 17, 2019
Merged

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

merged 2 commits into from
Apr 17, 2019

Conversation

ibezkrovnyi
Copy link
Contributor

@ibezkrovnyi ibezkrovnyi commented Apr 12, 2019

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
Contributor Author

ibezkrovnyi commented Apr 12, 2019

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 this to Needs Author Attention in Pull Request Status Board Apr 12, 2019
@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
Contributor

typescript-bot commented Apr 12, 2019

@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
Contributor

typescript-bot commented Apr 12, 2019

@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
Contributor Author

ibezkrovnyi commented Apr 13, 2019

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
Contributor

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
Contributor Author

@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
Contributor

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
Contributor Author

Removed non-consistent periods in JSDoc to trigger CI build

@typescript-bot typescript-bot moved this from Needs Author Attention to Check and Merge in Pull Request Status Board Apr 17, 2019
@typescript-bot
Copy link
Contributor

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
Pull Request Status Board automation moved this from Check and Merge to Done Apr 17, 2019
@typescript-bot
Copy link
Contributor

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
…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
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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants