[react-redux] Correctly infer prop types from wrapped component #28189

apapirovski · 2018-08-17T06:10:42Z

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Add or edit tests to reflect the change. (Run with npm test.)
Follow the advice from the readme.
Avoid common mistakes.
Run npm run lint package-name (or tsc if no tslint.json is present).
Provide a URL to documentation or source code which provides context for the suggested changes: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-0.html

This makes sure the inferred props include the defaultProps and propTypes resolutions by using JSX.LibraryManagedAttributes. I don't know how people feel about using JSX.LibraryManagedAttributes here directly but it feels better than rewriting the same thing from scratch...

This fixes #27959

typescript-bot · 2018-08-17T06:22:24Z

@apapirovski Thank you for submitting this PR!

🔔 @tkqubo @thasner @kenzierocks @clayne11 @tansongyang @NicholasBoll @mDibyo @pdeva @Kallikrein @val1984 @jrakotoharisoa - 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 · 2018-08-23T00:25:03Z

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!

DanielRosenwasser · 2018-08-24T23:02:45Z

🔔 @tkqubo @thasner @kenzierocks @clayne11 @tansongyang @NicholasBoll @mDibyo @pdeva @Kallikrein @val1984 @jrakotoharisoa can you take a look please?

fa-askin · 2018-08-30T09:37:09Z

types/react-redux/react-redux-tests.tsx

+	<ConnectedComponent fn={() => {}} />
+
+	const ConnectedComponent2 = connect<MapStateProps, void, OwnProps>(mapStateToProps)(Component);
+	<ConnectedComponent fn={() => {}} />


Shouldn't that be <ConnectedComponent2 fn={() => {}} />?

Yes! Will fix that.

apapirovski · 2018-09-02T00:55:24Z

@DanielRosenwasser Looks like the bot removed the Merge: YSYL label accidentally. Any chance it could be re-added and/or this PR reviewed? Thanks in advance!

types/react-redux/index.d.ts

apapirovski · 2018-09-11T22:20:27Z

@DanielRosenwasser @tkqubo @thasner @kenzierocks @clayne11 @tansongyang @NicholasBoll @mDibyo @pdeva @Kallikrein @val1984 @jrakotoharisoa Wondering if there's any chance I could get this reviewed? It's been a little while now. Thanks in advance!

octylFractal

Looks good, except for the comment on Matching.

octylFractal · 2018-09-11T22:57:04Z

types/react-redux/index.d.ts

+ * a property P will be present if:
+ * - it is present in DecorationTargetProps
+ * - it is present in InjectedProps[P] and can satisfy DecorationTargetProps[P]
+ *   or it is not present in InjectedProps[P]


I'm a little confused by this statement, it effectively says that P will be present if it is not present in InjectedProps?
This doesn't appear to match what the code does. Could this be cleared up?

The comment is not quite correct anymore for the recently updated version. Updating right now.

types/react-redux/index.d.ts

typescript-bot · 2018-09-11T23:26:30Z

@apapirovski One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you!

apapirovski · 2018-09-11T23:53:29Z

@kenzierocks I've updated the comment.

typescript-bot · 2018-09-12T00:26:36Z

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

apapirovski · 2018-09-12T00:33:31Z

I don't think the test is related to this PR :| See #28759

Make sure the inferred props include the defaultProps and propTypes resolutions by using JSX.LibraryManagedAttributes.

typescript-bot · 2018-09-12T19:27:46Z

🔔 @kenzierocks - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate?

apapirovski · 2018-09-13T15:15:41Z

@kenzierocks Any chance you have the ability to add/remove labels? This has the "revision needed" which makes it impossible to merge.

Otherwise, @DanielRosenwasser? Any chance you can remove the label?

octylFractal · 2018-09-13T15:32:29Z

I can't remove labels as a Contributor. You might try pushing again to re-run Travis, or if the failure is related to that PR, rebase off of master to incorporate the fix for it.

apapirovski · 2018-09-13T16:13:06Z

@kenzierocks Already reran Travis. I think the bot just has a bug around the "revision needed" label. Happened to me in another PR here a while ago. I guess we wait...

OliverJAsh · 2019-01-31T11:17:53Z

This PR has caused a few new bugs. Please can someone advise? We're not sure what the intention of Matching is: #31363 (comment)

apapirovski force-pushed the infer-prop-types-correctly branch from 663013b to 074d394 Compare August 17, 2018 06:21

typescript-bot added the Popular package This PR affects a popular package (as counted by NPM download counts). label Aug 17, 2018

typescript-bot added the Awaiting reviewer feedback label Aug 17, 2018

apapirovski force-pushed the infer-prop-types-correctly branch from 074d394 to 699d966 Compare August 18, 2018 00:11

typescript-bot added the Unmerged The author did not merge the PR when it was ready. label Aug 23, 2018

eps1lon mentioned this pull request Aug 27, 2018

[typescript] Fix with* injectors ignoring defaultProps mui/material-ui#12673

Merged

fa-askin reviewed Aug 30, 2018

View reviewed changes

typescript-bot removed the Unmerged The author did not merge the PR when it was ready. label Aug 30, 2018

typescript-bot added the Unmerged The author did not merge the PR when it was ready. label Sep 4, 2018

eps1lon reviewed Sep 7, 2018

View reviewed changes

types/react-redux/index.d.ts Outdated Show resolved Hide resolved

apapirovski force-pushed the infer-prop-types-correctly branch from a380c94 to beb67b4 Compare September 7, 2018 15:52

typescript-bot removed the Unmerged The author did not merge the PR when it was ready. label Sep 7, 2018

octylFractal suggested changes Sep 11, 2018

View reviewed changes

typescript-bot added Revision needed This PR needs code changes before it can be merged. and removed Awaiting reviewer feedback labels Sep 11, 2018

apapirovski force-pushed the infer-prop-types-correctly branch from bad79f8 to 7a30679 Compare September 11, 2018 23:58

typescript-bot added the The Travis CI build failed label Sep 12, 2018

typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Sep 12, 2018

apapirovski added 4 commits September 12, 2018 11:32

[react-redux] correctly infer prop types from wrapped component

dfbcd7c

Make sure the inferred props include the defaultProps and propTypes resolutions by using JSX.LibraryManagedAttributes.

fixup

781b676

fixup

2b4662d

adjust the comment

61092a2

apapirovski force-pushed the infer-prop-types-correctly branch from 7a30679 to 61092a2 Compare September 12, 2018 18:32

typescript-bot added Revision needed This PR needs code changes before it can be merged. and removed The Travis CI build failed labels Sep 12, 2018

octylFractal approved these changes Sep 12, 2018

View reviewed changes

typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. Merge:Express labels Sep 12, 2018

RyanCavanaugh merged commit ee59cca into DefinitelyTyped:master Sep 13, 2018

apapirovski deleted the infer-prop-types-correctly branch September 13, 2018 22:17

dan-kez mentioned this pull request Oct 8, 2018

[@types/styled-component] styled() does not recognize React defaultProps in Typescript 3 #29540

Closed

4 tasks

peterox mentioned this pull request Oct 24, 2018

[@types/react-redux] Breaking change in 6.0.9 #29991

Closed

4 tasks

OliverJAsh mentioned this pull request Dec 15, 2018

react-redux: type error for connect inside HOC #31363

Closed

4 tasks

noahm mentioned this pull request Aug 12, 2019

@types/react-redux The 'connect' function seems to be returning 'any' #37414

Closed

4 tasks

qnighy mentioned this pull request Jul 8, 2021

react-redux: fix wrapping components with propTypes #54363

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[react-redux] Correctly infer prop types from wrapped component #28189

[react-redux] Correctly infer prop types from wrapped component #28189

apapirovski commented Aug 17, 2018 •

edited

Loading

typescript-bot commented Aug 17, 2018 •

edited

Loading

typescript-bot commented Aug 23, 2018 •

edited

Loading

DanielRosenwasser commented Aug 24, 2018 •

edited

Loading

fa-askin Aug 30, 2018

apapirovski Aug 30, 2018

apapirovski commented Sep 2, 2018

apapirovski commented Sep 11, 2018

octylFractal left a comment

octylFractal Sep 11, 2018

apapirovski Sep 11, 2018

typescript-bot commented Sep 11, 2018

apapirovski commented Sep 11, 2018

typescript-bot commented Sep 12, 2018 •

edited

Loading

apapirovski commented Sep 12, 2018 •

edited

Loading

typescript-bot commented Sep 12, 2018

apapirovski commented Sep 13, 2018

octylFractal commented Sep 13, 2018

apapirovski commented Sep 13, 2018 •

edited

Loading

OliverJAsh commented Jan 31, 2019

[react-redux] Correctly infer prop types from wrapped component #28189

[react-redux] Correctly infer prop types from wrapped component #28189

Conversation

apapirovski commented Aug 17, 2018 • edited Loading

typescript-bot commented Aug 17, 2018 • edited Loading

typescript-bot commented Aug 23, 2018 • edited Loading

DanielRosenwasser commented Aug 24, 2018 • edited Loading

fa-askin Aug 30, 2018

Choose a reason for hiding this comment

apapirovski Aug 30, 2018

Choose a reason for hiding this comment

apapirovski commented Sep 2, 2018

apapirovski commented Sep 11, 2018

octylFractal left a comment

Choose a reason for hiding this comment

octylFractal Sep 11, 2018

Choose a reason for hiding this comment

apapirovski Sep 11, 2018

Choose a reason for hiding this comment

typescript-bot commented Sep 11, 2018

apapirovski commented Sep 11, 2018

typescript-bot commented Sep 12, 2018 • edited Loading

apapirovski commented Sep 12, 2018 • edited Loading

typescript-bot commented Sep 12, 2018

apapirovski commented Sep 13, 2018

octylFractal commented Sep 13, 2018

apapirovski commented Sep 13, 2018 • edited Loading

OliverJAsh commented Jan 31, 2019

apapirovski commented Aug 17, 2018 •

edited

Loading

typescript-bot commented Aug 17, 2018 •

edited

Loading

typescript-bot commented Aug 23, 2018 •

edited

Loading

DanielRosenwasser commented Aug 24, 2018 •

edited

Loading

typescript-bot commented Sep 12, 2018 •

edited

Loading

apapirovski commented Sep 12, 2018 •

edited

Loading

apapirovski commented Sep 13, 2018 •

edited

Loading