CPLAT-12469 Update to React 17 #277
Conversation
joebingham-wk
requested review from
kealjones-wk
and removed request for
kealjones-wk
* master: (46 commits) react 5.6.0 Add missing stuff to 5.5.1 changelog entry Don't check dartfmt when using the dev build of Dart Remove print statement Fix forwardRef2/useImperativeHandle typing, update tests to use forwardRef2 and test all ref types Rename test utility react 5.5.1 Add regression test for passing forwardRef2 into memo2 Reinstate tests for memo and forwardRef Address CR feedback related to comments useImperativeHandle should not throw if ref is null Run npm audit fix, build JS sources Remove unused ref un-conversion logic Improve doc comment Cleanup / comments Format Fix lints Clean up test case objects Improve forwardRef and base ref tests, fix chaining of converted refs Relocate similar event handler unconversion logic ...
js_src/_dart_helpers.js
Outdated
| // Must call checkPropTypes manually because React moved away from using the `prop-types` package. | ||
| // See: https://github.com/facebook/react/pull/18127 | ||
| // React now uses its own internal cache of errors for PropTypes which broke `PropTypes.resetWarningCache()`. | ||
| // Solution was to use `PropTypes.checkPropTypes` directly which makes `PropTypes.resetWarningCache()` work. | ||
| // Solution from: https://github.com/facebook/react/issues/18251#issuecomment-609024557 | ||
| // TODO: figure out how to get the `displayName` here... | ||
| React.PropTypes.checkPropTypes(jsConfig.propTypes, this.props, 'prop', this._getDisplayName()); | ||
| var result = dartInteropStatics.handleRender(this.dartComponent, this.props, this.state, this.context); | ||
| if (typeof result === 'undefined') result = null; | ||
| return result; | ||
| } | ||
| // Hacky workaround to get the displayName for `checkPropTypes` call. | ||
| _getDisplayName() { | ||
| return ReactDartComponent2.displayName; | ||
| } |
There was a problem hiding this comment.
It should be noted that I opted for this route to prevent breakages in the PropTypes api, specifically the resetWarningCache() react has opted to build in their own "prop-types" rather than using the external package. Currently react does not provide a way to reset the warning cache of itself, which we use quite a bit in tests, so i opted to use the external package. It is also unclear what the future holds for prop-types in general so the external package is a safer bet for long term support as the React team feels that Typescript on its own provides most of the prop type checks statically, unfortunately dart types do not support things such as Literal Types, Tuples, Unions, Optional/Required fields (i mean they kinda do but not as nicely), etc...
# Conflicts: # lib/react.js.map # lib/react_with_addons.js.map
|
Dart stable is failing from master, but |
js_src/_dart_helpers.js
Outdated
| // React now uses its own internal cache of errors for PropTypes which broke `PropTypes.resetWarningCache()`. | ||
| // Solution was to use `PropTypes.checkPropTypes` directly which makes `PropTypes.resetWarningCache()` work. | ||
| // Solution from: https://github.com/facebook/react/issues/18251#issuecomment-609024557 | ||
| React.PropTypes.checkPropTypes(jsConfig.propTypes, this.props, 'prop', ReactDartComponent2.displayName); |
There was a problem hiding this comment.
Two things:
- can we wrap this in a compile-time check so that this call isn't present in the dev bundle?
| React.PropTypes.checkPropTypes(jsConfig.propTypes, this.props, 'prop', ReactDartComponent2.displayName); | |
| if (process.env.NODE_ENV !== 'production') { | |
| React.PropTypes.checkPropTypes(jsConfig.propTypes, this.props, 'prop', ReactDartComponent2.displayName); | |
| } |
- Can we update the propTypes tests to verify that the correct displayName is being passed? RIght now we capture all of the information passed into prop validators but only test some of it
There was a problem hiding this comment.
It seems like the displayName is being tested here? Do you want me to add that check in both places?
There was a problem hiding this comment.
Oh good catch! It looks like that test would pass if the name is ReactDartComponent2, though, and we wouldn't be able to tell if the wrong name is being passed in.
Now that it's wrapped in that assertsEnabled() check, though (which wasn't present when the original test was written), I think that case shouldn't happen, and we could update it like so:
-'componentName': anyOf('PropTypesTestComponent', 'ReactDartComponent2'),
+'componentName': 'PropTypesTestComponent',and then we'd know for sure that the test name is being passed through.
Would you mind updating that test so that we get full coverage on this code?
There was a problem hiding this comment.
Updated in 2978197!
setReactConfiguration- sourcegraphsetReactDOMConfiguration- sourcegraphReactDartComponentFactoryProxy.reactComponentFactoryandReactDartComponentFactoryProxy2.reactComponentFactory- sourcegraphReactJsContextComponentFactoryProxy.factory,ReactJsComponentFactoryProxy.factory, andReactDomComponentFactoryProxy.factory- sourcegraphQA Instructions
Please review: @joebingham-wk @aaronlademann-wf @greglittlefield-wf