[createReactClass] remove createReactClass from RNTester/js/ImageExample.js #21602
Conversation
facebook-github-bot
added
the
CLA Signed
RSNara
changed the title
There was a problem hiding this comment.
Looks mostly good, but I think we should make a few changes. 🧐
I think there's a lot of cases where we're using the source prop type. It's copy-pasted 4 times. It'd probably be better if we pull it out into its own type and re-use it. Something like:
type ImageSource = $ReadOnly<|uri: string|>;Since we're forwarding the source to the Image component anyway, we could (or maybe we should) just also do:
type ImageProps = React.ElementProps<typeof Image>;
type ImageSource = $ElementType<ImageProps, 'source'>
RNTester/js/ImageExample.js
Outdated
| }; | ||
|
|
||
| type NetworkImageCallbackExampleState = { | ||
| events: string[], |
There was a problem hiding this comment.
nit: For consistency, we should use: Array<string>.
RNTester/js/ImageExample.js
Outdated
| }, | ||
| prefetchedSource: { | ||
| uri: string, | ||
| }, |
There was a problem hiding this comment.
Flow's $ReadOnly (like Object.freeze) only goes 1 layer deep. We should probably change this to:
type NetworkImageCallbackExampleProps = $ReadOnly<{|
source: $ReadOnly<{|
uri: string,
|}>,
prefetchedSource: $ReadOnly<{|
uri: string,
|}>,
|}>;There was a problem hiding this comment.
Thank you!!
I didn't understand Flow's $ReadOnly (like Object.freeze) only goes 1 layer deep
RNTester/js/ImageExample.js
Outdated
| NetworkImageCallbackExampleProps, | ||
| NetworkImageCallbackExampleState, | ||
| > { | ||
| static displayName: ?string = 'NetworkImageCallbackExample'; |
RNTester/js/ImageExample.js
Outdated
| }; | ||
|
|
||
| type ImageSizeExampleProps = $ReadOnly<{| | ||
| source: { |
There was a problem hiding this comment.
type ImageSizeExampleProps = $ReadOnly<{|
source: $ReadOnly<{|
uri: string,
|}>
|}>
RNTester/js/ImageExample.js
Outdated
| }; | ||
|
|
||
| class MultipleSourcesExample extends React.Component< | ||
| {}, |
There was a problem hiding this comment.
nit: Can we move this out to MultipleSourcesExampleProps?
There was a problem hiding this comment.
RSNara has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@nd-02110114 merged commit 897f721 into |
Summary: Related to #21581 . Removed createReactClass from the RNTester/js/ImageExample.js The diff of this PR is a little big. If there are any problems, please teach me � - [x] npm run prettier - [x] npm run flow-check-ios - [x] npm run flow-check-android [GENERAL] [ENHANCEMENT] [RNTester/js/ImageExample.js] - remove createReactClass dependency Pull Request resolved: facebook/react-native#21602 Reviewed By: TheSavior Differential Revision: D10304857 Pulled By: RSNara fbshipit-source-id: 339b1220828c6218cad0d09c7a5034a61e623bc6
) Summary: Related to facebook#21581 . Removed createReactClass from the RNTester/js/ImageExample.js The diff of this PR is a little big. If there are any problems, please teach me � - [x] npm run prettier - [x] npm run flow-check-ios - [x] npm run flow-check-android [GENERAL] [ENHANCEMENT] [RNTester/js/ImageExample.js] - remove createReactClass dependency Pull Request resolved: facebook#21602 Reviewed By: TheSavior Differential Revision: D10304857 Pulled By: RSNara fbshipit-source-id: 339b1220828c6218cad0d09c7a5034a61e623bc6
Related to #21581 .
Removed createReactClass from the RNTester/js/ImageExample.js
The diff of this PR is a little big. If there are any problems, please teach me 🙇
Test Plan
Release Notes
[GENERAL] [ENHANCEMENT] [RNTester/js/ImageExample.js] - remove createReactClass dependency