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

[react] Port ElementRef utility type from Flow #43201

Merged
merged 10 commits into from Mar 20, 2020

Conversation

alloy
Copy link
Collaborator

@alloy alloy commented Mar 18, 2020

Flow doc: https://flow.org/en/docs/react/types/#toc-react-elementref

In short, this utility type makes it easy to get the type a ref would have, given a component type. While not-essential, it will make keeping up-to-date with upstream React code simpler; or more specifically react-native.

I am currently introducing it in a test for the react-native typings.

types/react/global.d.ts Outdated Show resolved Hide resolved
types/react/global.d.ts Outdated Show resolved Hide resolved
@alloy alloy force-pushed the react-add-element-ref-utility branch from 2f874a4 to 6656d6f Compare March 18, 2020 14:12
types/react/global.d.ts Outdated Show resolved Hide resolved
@alloy alloy requested a review from andrewbranch March 18, 2020 14:16
@alloy alloy mentioned this pull request Mar 18, 2020
9 tasks
@typescript-bot typescript-bot added this to Waiting for Reviewers in Pull Request Status Board Mar 18, 2020
@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Awaiting reviewer feedback labels Mar 18, 2020
@typescript-bot
Copy link
Contributor

typescript-bot commented Mar 18, 2020

@alloy Thank you for submitting this PR!

🔔 @johnnyreilly @bbenezech @pzavolinsky @digiguru @ericanderson @DovydasNavickas @theruther4d @guilhermehubner @ferdaber @jrakotoharisoa @pascaloliv @Hotell @franklixuefei @Jessidhia @saranshkataria @lukyth @eps1lon @zieka @dancerphil @dimitropoulos @rapmue @royxue @ZheyangSong @richbai90 @caspeco-dan @pkeuter @jrsaunde @CruseCtrl @apalugniok @RobertStigsson @kousaku-maron @iflp @veddermatic @G07cha @gndelia @eliotball @vkentta @fcaylus - 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.

@alloy
Copy link
Collaborator Author

alloy commented Mar 18, 2020

cc @TheSavior

Copy link
Collaborator

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Do we really need this in the core typings or is this userland implementation sufficient:

// helper
type RefInstance<T> = T extends React.Ref<infer InstanceType> ? InstanceType : never;
// usage
type AnchorRef = NonNullable<React.ComponentPropsWithRef<'a'>['ref']>;
type AnchorInstance = RefInstance<AnchorRef>;

@alloy
Copy link
Collaborator Author

alloy commented Mar 18, 2020

@eps1lon Like I mentioned, this is not essential to the core typings, but it does make it much simpler to port upstream typings. Thus far these are most (all?) react-native related, so I’m ok moving it there; but in principle [in upstream] this is a type re-exported from the react package.

@eps1lon
Copy link
Collaborator

eps1lon commented Mar 18, 2020

My main concern is that big type map. This seems solvable by the type helper I posted, no?

@typescript-bot typescript-bot moved this from Waiting for Reviewers to Needs Author Attention in Pull Request Status Board Mar 18, 2020
@typescript-bot
Copy link
Contributor

@alloy 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!

@alloy alloy force-pushed the react-add-element-ref-utility branch from 6656d6f to 75fae5c Compare March 18, 2020 15:36
@alloy
Copy link
Collaborator Author

alloy commented Mar 18, 2020

@eps1lon Gotcha 👍 I just got rid of it, albeit slightly different. Let me know if you still have concerns.

@alloy alloy marked this pull request as ready for review March 18, 2020 15:41
@alloy alloy force-pushed the react-add-element-ref-utility branch from 75fae5c to de1592a Compare March 18, 2020 15:48
@alloy
Copy link
Collaborator Author

alloy commented Mar 18, 2020

Hmm, there’s a typescript@next error that I don’t see how it could be related to my change: https://travis-ci.org/github/DefinitelyTyped/DefinitelyTyped/builds/664007029#L892. Previous React PR was green, though, so something must have changed since then.

@ferdaber
Copy link
Contributor

This doesn't work for forwardref exotic components either.

@typescript-bot typescript-bot moved this from Needs Author Attention to Waiting for Reviewers in Pull Request Status Board Mar 18, 2020
types/react/index.d.ts Outdated Show resolved Hide resolved
types/react/test/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@ferdaber ferdaber left a comment

Choose a reason for hiding this comment

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

Thanks for contributing!

@alloy
Copy link
Collaborator Author

alloy commented Mar 19, 2020

@ferdaber et al, thanks for the great review! 🙏

@typescript-bot
Copy link
Contributor

typescript-bot commented Mar 19, 2020

@alloy 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!

@alloy
Copy link
Collaborator Author

alloy commented Mar 20, 2020

The remaining CI failure has to do with a property being removed from TS’ lib.dom.d.ts declaration file. At the time of writing, this is v3.9.0-dev.20200320. Weirdly enough when I check the master branch it does exist https://github.com/microsoft/TypeScript/blob/5e9c43607f8732bc94374c95fddd8b1fd9881122/lib/lib.dom.d.ts#L2907

Perhaps this file gets updated during the release process?

In any case, it’s a failure unrelated to my changes.

@ferdaber
Copy link
Contributor

Can you tag some of the maintainers for recharts for this? So we can raise the issue to them.

@alloy
Copy link
Collaborator Author

alloy commented Mar 20, 2020

Ok, found the place it was removed and tagged the maintainers there https://github.com/microsoft/TypeScript/pull/37464/files#r395692943

@orta
Copy link
Collaborator

orta commented Mar 20, 2020

Re:

ERROR: 148:99  expect  TypeScript@next compile error: 
2202Type 'PickedCSSStyleDeclarationKeys' does not satisfy the constraint 'number | "length" | "all" | "left" | "right" | "bottom" | "top" | "font" | "clipPath" | "filter" | "marker" | "mask" | "resize" | "color" | "clip" | "content" | "flex" | "grid" | ... 370 more ... | "setProperty"'.
2203  Type '"enableBackground"' is not assignable to type 'number | "length" | "all" | "left" | "right" | "bottom" | "top" | "font" | "clipPath" | "filter" | "marker" | "mask" | "resize" | "color" | "clip" | "content" | "flex" | "grid" | ... 370 more ... | "setProperty"'.
2204
2205

I'd recommend just dropping that property from the types, it's an IE only property which was deprecated years ago

Copy link
Contributor

@ferdaber ferdaber left a comment

Choose a reason for hiding this comment

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

Going above and beyond 👍

@typescript-bot

This comment has been minimized.

@orta
Copy link
Collaborator

orta commented Mar 20, 2020

Alright, this looks good 👍

@orta orta merged commit 67fdf14 into DefinitelyTyped:master Mar 20, 2020
Pull Request Status Board automation moved this from Needs Author Attention to Done Mar 20, 2020
@alloy alloy deleted the react-add-element-ref-utility branch March 20, 2020 17:45
@typescript-bot
Copy link
Contributor

I just published @types/react@16.9.25 to npm.

@typescript-bot
Copy link
Contributor

I just published @types/recharts@1.8.9 to npm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Popular package This PR affects a popular package (as counted by NPM download counts). Revision needed This PR needs code changes before it can be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants