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

Creating Standalone JS File For Each Native Component #22990

Closed
22 tasks done
elicwhite opened this issue Jan 15, 2019 · 36 comments
Closed
22 tasks done

Creating Standalone JS File For Each Native Component #22990

elicwhite opened this issue Jan 15, 2019 · 36 comments
Labels
Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Help Wanted :octocat: Issues ideal for external contributors. JavaScript Resolution: Locked This issue was locked by the bot.

Comments

@elicwhite
Copy link
Member

elicwhite commented Jan 15, 2019

We are going to be generating native code from flow types in JS for our native components. In order to do this we need to have an individual JS file for each native component with complete flow types.

Step 1, Step 2, Step 3, and Step 4 have all been steps towards this goal.

In a bunch of our JS files we choose a component based on platform, typically looking something like this:

const SwitchNativeComponent: SwitchNativeComponentType =
  Platform.OS === 'android'
    ? (requireNativeComponent('AndroidSwitch'): any)
    : (requireNativeComponent('RCTSwitch'): any);

we want to change this code so that each requireNativeComponent call lives in its own file, thus changing the above code to look like this:

const SwitchNativeComponent: SwitchNativeComponentType =
  Platform.OS === 'android'
    ? require('AndroidSwitchNativeComponent')
    : require('RCTSwitchNativeComponent');

We need your help to make this change!

How to help

Pick one of the native components from the list below. Comment that you are working on that file and try to choose one that nobody else has commented about already.

Search the codebase for .js files that include that name. We are specifically looking for requireNativeComponent calls. So if you pick the component AndroidCheckBox we'd be looking for this callsite.

Create a new js file suffixed with 'NativeComponent' right next to the file that you found. In this case we'd create a AndroidCheckBoxNativeComponent.js file in Libraries/Components/CheckBox.

We should move the call to requireNativeComponent from Checkbox.android.js to this file, including the flow type if one exists, or defining one if it doesn't.

A pastebin of what I think the changes look like for this component are here: https://pastebin.com/RFpdT76V

One thing to note is that the majority of types necessary for these files shouldn't be imported from other files. Our codegen can't yet handle importing types from other files so in most cases I'd prefer to duplicate the type in this new file if it is needed. There are some exceptions to this that the codegen can handle that I'd expect most of these files will need. Specifically:

import type {ViewProps} from 'ViewPropTypes';
import type {SyntheticEvent} from 'CoreEventTypes';
import type {NativeComponent} from 'ReactNative';

If you come across other types that you think are sufficiently complex that you think shouldn't be duplicated tag me on your PR. Perhaps we should be adding knowledge of that type to the codegen. 😄

Testing Your Changes

For the highest confidence in your change, we recommend testing your changes using the RNTester App. Instructions to install and test the app are in the readme in the RNTester directory. If your component is an Android component you should test it on Android. If it is iOS, then test on iOS. 😄

The Files

  • AndroidCheckBox
  • AndroidDialogPicker
  • AndroidDrawerLayout
  • AndroidDropdownPicker
  • AndroidProgressBar
  • AndroidSwipeRefreshLayout
  • AndroidViewPager
  • RCTActivityIndicatorView
  • RCTDatePicker
  • RCTInputAccessoryView
  • RCTMaskedView
  • RCTModalHostView
  • RCTPicker
  • RCTProgressView
  • RCTRefreshControl
  • RCTSafeAreaView
  • RCTSegmentedControl
  • RCTSlider
  • RCTSnapshot
  • RCTTabBar
  • RCTTabBarItem
  • ToolbarAndroid
@elicwhite elicwhite added Good first issue Interested in collaborating? Take a stab at fixing one of these issues. JavaScript Help Wanted :octocat: Issues ideal for external contributors. 🔶Components labels Jan 15, 2019
@nissy-dev
Copy link
Contributor

nissy-dev commented Jan 15, 2019

I'll take AndroidCheckBox

@Naturalclar
Copy link
Contributor

I'd like to contribute! I'll work on AndroidDropdownPicker

@ni3t
Copy link
Contributor

ni3t commented Jan 15, 2019

Hello! RCTDatePicker

@Kriyszig
Copy link

Would love to help with AndroidViewPager

@vickywane
Copy link

I'll take RCTTabBar

@ajstrand
Copy link

I'll take the AndroidProgressBar.

@Chi-AnTai
Copy link
Contributor

I will try to help with RCTPicker

@Illu
Copy link
Contributor

Illu commented Jan 15, 2019

Hello! I will take the RCTSafeAreaView

@OdaDaisuke
Copy link

OdaDaisuke commented Jan 15, 2019

Hello! I’ll work on RCTSlider

@stu60610
Copy link
Contributor

I'll take the RCTSegmentedControl

@sasurau4
Copy link
Contributor

I'll take the ToolbarAndroid

@Leko
Copy link
Contributor

Leko commented Jan 15, 2019

I'll take the RCTMaskedView

@a-dilettante
Copy link
Contributor

I'll take the RCTProgressView 😄

@leanmazzu
Copy link

I'll take AndroidDialogPicker 🙋🏻‍♂️

@Leko
Copy link
Contributor

Leko commented Jan 15, 2019

I'll work on RCTTabBarItem.

@cpojer cpojer closed this as completed Jan 23, 2019
facebook-github-bot pushed a commit that referenced this issue Jan 23, 2019
Summary:
Changelog:
----------
[iOS][Changed] - added types to RCTSnapshotNativeComponent.js, as mentioned in #22990
Pull Request resolved: #23111

Differential Revision: D13781429

Pulled By: cpojer

fbshipit-source-id: 7efffe150fd29cbfbb3a6b8f13e38295f83acb3c
facebook-github-bot pushed a commit that referenced this issue Jan 23, 2019
Summary:
PR Related to: #22990

Changelog:
[Android][Changed] - All the imports connected to requireNativeComponent in ActivityIndicator was moved to  a seperate file.
Pull Request resolved: #23104

Differential Revision: D13781451

Pulled By: cpojer

fbshipit-source-id: 7204976d59a96abdaa81cdd7fd54fd001f7d1ee9
@reemtariqq
Copy link

i think all is taken right ? :)

@doniyor2109
Copy link
Contributor

@reemtariqq Unfortunately all are taken. However you can search for issues with "Good first issue" label. There are many of them

matt-oakes pushed a commit to matt-oakes/react-native that referenced this issue Feb 7, 2019
Summary:
Part of facebook#22990

Changelog:
----------
[iOS] [Changed] - Create individual ```RCTSnapshotNativeComponent``` JS file from ```SnapshotViewIOS```
Pull Request resolved: facebook#23012

Differential Revision: D13683566

Pulled By: PeteTheHeat

fbshipit-source-id: d7a030dd59e9c5dd88f7ddd92734486063c5f450
matt-oakes pushed a commit to matt-oakes/react-native that referenced this issue Feb 7, 2019
Summary:
This PR is related to facebook#22990

Changelog:
----------
[Android][Changed]  - move the call to requireNativeComponent from `Checkbox.android.js` to `AndroidCheckBoxNativeComponent.js`
Pull Request resolved: facebook#23003

Reviewed By: TheSavior

Differential Revision: D13690827

Pulled By: rickhanlonii

fbshipit-source-id: 08bc83a7f097414b5c833a3b43715e5aec277d65
matt-oakes pushed a commit to matt-oakes/react-native that referenced this issue Feb 7, 2019
Summary:
Part of facebook#22990

Changelog:
----------

[iOS] [Changed] - Create individual `RCTSafeAreaViewNativeComponent` JS file from `SafeAreaView` with flow typing.
Pull Request resolved: facebook#23006

Reviewed By: TheSavior

Differential Revision: D13690683

Pulled By: rickhanlonii

fbshipit-source-id: 7928fabf0264a6269a41148432f604e82d9b3920
matt-oakes pushed a commit to matt-oakes/react-native that referenced this issue Feb 7, 2019
Summary:
This pull request is part of facebook#22990.

Changelog:
----------

[iOS] [Changed] - Split RCTTabBarItem into RCTTabBarItemNativeComponent.
Pull Request resolved: facebook#23004

Reviewed By: TheSavior

Differential Revision: D13690820

Pulled By: rickhanlonii

fbshipit-source-id: 522e55ee74a797a3930edf6821c6d8f72511cd2b
matt-oakes pushed a commit to matt-oakes/react-native that referenced this issue Feb 7, 2019
Summary:
Changelog:
----------

[Android] [Changed] - As mentioned in facebook#22990, I have moved native components required by PickerAndroid.android.js into separate files and added Flow Typing.
Pull Request resolved: facebook#22999

Differential Revision: D13697130

Pulled By: TheSavior

fbshipit-source-id: 42da73d82cca45fefa066871eed5a637811643c8
matt-oakes pushed a commit to matt-oakes/react-native that referenced this issue Feb 7, 2019
Summary:
This pull request is part of facebook#22990.

Changelog:
----------

[iOS] [Changed] - Split RCTMaskedView into RCTMaskedViewNativeComponent.
Pull Request resolved: facebook#23001

Differential Revision: D13697245

Pulled By: TheSavior

fbshipit-source-id: 16af0b394ae32cd3c4992c2cd5ea2d3c140755b3
matt-oakes pushed a commit to matt-oakes/react-native that referenced this issue Feb 7, 2019
Summary:
Thank you for sending the PR! We appreciate you spending the time to work on these changes.
Help us understand your motivation by explaining why you decided to make this change:

Changelog:
----------

[iOS] [Changed] - As mentioned in facebook#22990, I have moved native components required by DatePickerIOS.ios.js into separate files and added Flow Typing.
Pull Request resolved: facebook#23013

Differential Revision: D13697591

Pulled By: TheSavior

fbshipit-source-id: 5aec5a2270cbfc708f3e3a67662abd8071f1333f
matt-oakes pushed a commit to matt-oakes/react-native that referenced this issue Feb 7, 2019
Summary:
[Android] [Changed] - create standalone JS file for ToolbarAndroid, as part of facebook#22990

I have a question.

`ToolbarAndroid` component use 2 types from other files.
They are not included files mentioned by facebook#22990 (comment).
One type `ColorValue` is very simple and I'll copy it.
But `ImageSource` type is a bit complicated I think it wolud be better to add to the codegen.
How do you think about it? TheSavior
Update: -> solved 👍

----------
- [x] yarn lint
- [x] yarn flow-check-android
- [x] yarn flow
- [x] yarn test
Pull Request resolved: facebook#23009

Differential Revision: D13697524

Pulled By: TheSavior

fbshipit-source-id: 72ce43e1510be0986a3798d795f07ab176bd2a92
matt-oakes pushed a commit to matt-oakes/react-native that referenced this issue Feb 7, 2019
Summary:
Changelog:
----------

[iOS] [Changed] -  follow facebook#22990, move the call to requireNativeComponent from `SegmentedControlIOS.ios.js` to `RCTSegmentedControlNativeComponent.js`
Pull Request resolved: facebook#23000

Differential Revision: D13697168

Pulled By: TheSavior

fbshipit-source-id: 83a66279d2fcd5a29d2ebc9cf5b5273947d96281
matt-oakes pushed a commit to matt-oakes/react-native that referenced this issue Feb 7, 2019
Summary:
This PR is related to facebook#22990

Changelog:
----------
[iOS] [Changed] - move the call to requireNativeComponent from Modal.js to RCTModalHostViewNativeComponent.js
Pull Request resolved: facebook#23030

Differential Revision: D13710032

Pulled By: cpojer

fbshipit-source-id: 822284a639f38721442c67ceff98fc99495c31b9
matt-oakes pushed a commit to matt-oakes/react-native that referenced this issue Feb 7, 2019
Summary:
Changelog:
----------

[iOS] [Changed] - moved RCTRefreshControl from RefreshControl as a separate component, as mentioned in facebook#22990
[Android] [Changed] - moved AndroidSwipeRefreshLayout from RefreshControl as a separate component, as mentioned in facebook#22990
Pull Request resolved: facebook#23039

Reviewed By: rickhanlonii

Differential Revision: D13710076

Pulled By: cpojer

fbshipit-source-id: 332520b74d6fc73e50dbe511dae22f82015c2d3a
matt-oakes pushed a commit to matt-oakes/react-native that referenced this issue Feb 7, 2019
Summary:
Changelog:
----------

[Android] [Changed] - As mentioned in facebook#22990, I have moved native components required by DrawerLayoutAndroid.android.js into separate files and added Flow Typing.

Question
----------
I have two questions.

It is not included in the files mentioned by facebook#22990 [comment](facebook#22990 (comment)). Perhaps we should be adding knowledge of that type to the codegen since it is quite complicated, what do you think TheSavior?

The `Props` type include `renderNavigationView: () => React.Element<any>,` and `children?: React.Node,`. Therefore I added `const React = require('React');` to the file, is it ok?
Pull Request resolved: facebook#23036

Differential Revision: D13710035

Pulled By: cpojer

fbshipit-source-id: 671423b76c3b443d85e4b63d05d6253dbd33b29c
matt-oakes pushed a commit to matt-oakes/react-native that referenced this issue Feb 7, 2019
Summary:
Created Standalone JS file for RCTInputAccessoryView native component facebook#22990

Changelog:
----------

[iOS] [Changed] - Created Standalone JS file for RCTInputAccessoryView native component
Pull Request resolved: facebook#23050

Differential Revision: D13735644

Pulled By: TheSavior

fbshipit-source-id: 64b091957b38cb11d804582f185d5cb0c6754af3
matt-oakes pushed a commit to matt-oakes/react-native that referenced this issue Feb 7, 2019
Summary:
[iOS] [Changed] - As facebook#22990 said, move requireNativeComponent to a separate file.
I am not familiar with flow, I try to follow the https://pastebin.com/RFpdT76V example but I am not sure I have done it right.
Pull Request resolved: facebook#22996

Differential Revision: D13697082

Pulled By: cpojer

fbshipit-source-id: c0c87a8e1a7f0553da994aba230f69b496140200
matt-oakes pushed a commit to matt-oakes/react-native that referenced this issue Feb 7, 2019
Summary:
his PR is related to facebook#22990

Changelog:
----------

[Android][Changed] - move the call to requireNativeComponent from ProgressBarAndroid.android.js to ProgressBarAndroidNativeComponent.js
Pull Request resolved: facebook#23068

Differential Revision: D13760445

Pulled By: cpojer

fbshipit-source-id: b74ff42c6f207803de70be549a13487d59125a66
matt-oakes pushed a commit to matt-oakes/react-native that referenced this issue Feb 7, 2019
Summary:
Changelog:

[iOS] [Changed] - Deal with facebook#22990. Move `requireNativeComponent` to a separate file.
Pull Request resolved: facebook#23048

Differential Revision: D13760373

Pulled By: cpojer

fbshipit-source-id: ff8cc9d468dc3bac55fc3d4156ad695dcdd10ab8
matt-oakes pushed a commit to matt-oakes/react-native that referenced this issue Feb 7, 2019
Summary:
Created a standalone JS file for the `RCTProgressView` native component.

This PR is part of facebook#22990.

Changelog:
----------

[iOS] [Changed] - Created a standalone JS file for the `RCTProgressView` native component
Pull Request resolved: facebook#23077

Differential Revision: D13760036

Pulled By: cpojer

fbshipit-source-id: 0f449528b28fde089d9c6b0eb9b752dee3a85af6
matt-oakes pushed a commit to matt-oakes/react-native that referenced this issue Feb 7, 2019
Summary:
[Android][Changed] - All the imports connected to `requireNativeComponent` in `ViewPager` was moved to  a separate file.
Issue in focus: facebook#22990
Pull Request resolved: facebook#22995

Differential Revision: D13760459

Pulled By: cpojer

fbshipit-source-id: fca1633ce933ea4909ef81d7bbe8123d654e24fb
matt-oakes pushed a commit to matt-oakes/react-native that referenced this issue Feb 7, 2019
Summary:
His PR is related to facebook#22990

Changelog:
----------

[IOS][Changed] - move the call to requireNativeComponent from TabBarIOS.ios.js to RCTTabBarNativeComponent.js
Pull Request resolved: facebook#23118

Differential Revision: D13781428

Pulled By: cpojer

fbshipit-source-id: 3034c7db127a992c5757d70e22d98ee9acf4847b
matt-oakes pushed a commit to matt-oakes/react-native that referenced this issue Feb 7, 2019
Summary:
Changelog:
----------
[iOS][Changed] - added types to RCTSnapshotNativeComponent.js, as mentioned in facebook#22990
Pull Request resolved: facebook#23111

Differential Revision: D13781429

Pulled By: cpojer

fbshipit-source-id: 7efffe150fd29cbfbb3a6b8f13e38295f83acb3c
matt-oakes pushed a commit to matt-oakes/react-native that referenced this issue Feb 7, 2019
Summary:
PR Related to: facebook#22990

Changelog:
[Android][Changed] - All the imports connected to requireNativeComponent in ActivityIndicator was moved to  a seperate file.
Pull Request resolved: facebook#23104

Differential Revision: D13781451

Pulled By: cpojer

fbshipit-source-id: 7204976d59a96abdaa81cdd7fd54fd001f7d1ee9
@RSNara
Copy link
Contributor

RSNara commented May 16, 2019

Thank you guys for all the work here! We have another opportunity to contribute to an RN project: TurboModules. Please check out #24875.

@facebook facebook locked as resolved and limited conversation to collaborators Jan 23, 2020
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jan 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Help Wanted :octocat: Issues ideal for external contributors. JavaScript Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests