-
Notifications
You must be signed in to change notification settings - Fork 127
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
Move iOS and Android Specific Code to Their Own Packages #49
base: main
Are you sure you want to change the base?
Move iOS and Android Specific Code to Their Own Packages #49
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this proposal!
Cc @necolas, @vincentriemer, @matthargett for context on some out of tree platforms.
I’m not sure what the implications are here for more internal pieces like the build system.
What might also be valuable is laying out how you think a change like this could be made. What are the necessary steps and the order of doing it. Also, trying to identify which pieces would require Facebook to complete vs being able to be done by the community.
import {AppRegistry, Alert, View} from 'react-native'; | ||
``` | ||
|
||
Platform-specific components and APIs would be imported from the platform's module: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about things that should be cross platform but just haven’t been implemented on the other platform yet? Do they start in a platform specific package and move into the core package later?
Or is it more based on the particular api/component in which case there would still be some things in core that don’t work on both platforms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about things that should be cross platform but just haven’t been implemented on the other platform yet?
I was thinking that an UnimplementedView
stub should be in the main package and an implemented JS file would be in the platform module.
Like in the case of CheckBox
, a UnimplementedView
stub CheckBox.js
would be in the main package, and a CheckBox.android.js
in react-native-android
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the core should be slim enough that its pretty much required to have it all implemented. So for instance, checkbox wouldn't be part of core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, would be good to keep core to a minimum "This is what you need to implement to be considered a full React Native implementation". Other stuff could be implemented by the platform implementers by submitting pull requests to the Slimmen'd repos (like what will probably happen with Windows' WebView implementation)
|
||
Platform-specific APIs could drop the platform suffix from their name. `PermissionsAndroid` could just be `Permissions` for example. | ||
|
||
React Native being a Yarn workspace might make it easier to make other modules that should be developed and released alongside the main project. It would be easier to make a more advanced RNTester app in-tree with its own set of dependencies, for example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RNTester being its own package in-tree is unrelated and could happen anyways right? Perhaps that should be part of the RNTester proposal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if that could be done without re-arranging it into a monorepo first, Yarn requires the top level package.json
of a repo to be private in a yarn workspace.
But to implement this idea, I was thinking I would first do a "smaller" pull request that just sets up a yarn workspace and nothing else, and that could get done sooner. Maybe the yarn workspace could be a separate proposal this one is dependant on?
* `react-native-ios` | ||
* **Components** | ||
* `DatePickerIOS` | ||
* `InputAccessoryView` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an example of something I could imagine being standard across many platforms. Also, should probably be pulled out with slimmening regardless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will move this to the main package when I revise this 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I think this particular one should just get moved to a community rep. But that is a conversation for slimmening. Nothing to worry about for this proposal.
} | ||
``` | ||
|
||
Platform-specific Flow prop types will probably need to stay bundled together in the main package. Ideally it could be set up so if you didn't have `react-native-ios` installed the iOS-only View props wouldn't show up when typechecking, for example. But I don't know if there's a way to do that with Flow yet. Would need [this issue](https://github.com/facebook/flow/issues/396) to be resolved, probably. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do other platforms like windows extend the props of components to provide platform specific props?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case of RNWindows they use the platform-overridable prop-types
files:
https://github.com/Microsoft/react-native-windows/blob/master/Libraries/Components/View/PlatformViewPropTypes.windows.js
https://github.com/Microsoft/react-native-windows/blob/master/Libraries/Text/TextPropTypes.windows.js
They don't seem to use platform overridable flow props anywhere. But a platform-specific X-Types.platform.js
would be awkward, because you wouldn't be able to see the flow types for windows
when flow checking *.ios.js
files for example.
|
||
There could be "bridge" versions of the `react-native-{ios,android}` modules for pre-split versions of React Native that simply import platform specific stuff from `react-native`. This would make it easier for people to make React Native addons that cover wider ranges of RN versions | ||
|
||
For a release or so, `react-native-{ios,android}` should probably be thin modules that just re-import stuff from `react-native` before the code actually gets moved over, to help people transition. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or the other way where React Native keeps exposing things from the native packages but the changes can be made in the native packages directly.
I think that the first thing that should be done is land an initial set up of a Yarn workspace. It would both reveal possible places internally where things might break, and would also make it so RNTester could sit in the workspace without being dependant on the progress/acceptance of this proposal. I will look into doing a PR of this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm against this, we should embrace unification of components in userland.
Components should render/work anywhere
Do you mean, like, moving all platform-specific components into userland instead of splitting them out into platform modules? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting together this proposal, I like it!
I think it's important to retain a large pool of "standard" components in RN to ensure a common foundation to develop upon. Something like this could be a good step towards making explicit separations between different concerns in React Native. And I think it's needed if support for other platforms is to eventually be integrated.
As someone who maintains react-native-web, the topic of iOS/Android-only components comes up a lot, and part of the problem is that people using RN can import Android/iOS-only components in a file that gets packaged to run on a different platform. But importing from react-android
, for example, would make it more obvious that there should be problem with trying to run Android code on iOS, Web, Windows, etc. We could consider making it a warning in RN for now, and eventually build error, so that the division between cross-platform and single-platform code is stricter.
|
||
* `react-native-ios` | ||
* **Components** | ||
* `DatePickerIOS` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the date pickers be made cross-platform?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DatePickerIOS
is a component, while DatePickerAndroid
is an API that pops up a little modal. Might be possible, but would be a bit of work
* `DatePickerIOS` | ||
* `InputAccessoryView` | ||
* `MaskedViewIOS` | ||
* `PickerIOS` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecate in favour of Picker
* `InputAccessoryView` | ||
* `MaskedViewIOS` | ||
* `PickerIOS` | ||
* `ProgressViewIOS` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ProgressViewIOS and ProgressBarAndroid could be deprecated in favour of a cross-platform ProgressBar (there's one in React Native for Web)
* `TabBarIOS` | ||
* **APIs** | ||
* `ActionSheetIOS` | ||
* `AlertIOS` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecated
* `ImagePickerIOS` | ||
* `PushNotificationIOS` | ||
* `StatusBarIOS` | ||
* `VibrationIOS` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecated for Vibration
* `ProgressBarAndroid` | ||
* `ToastAndroid` | ||
* `ToolbarAndroid` | ||
* `ViewPagerAndroid` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be made cross-platform
----📁 third-party/ | ||
----📁 tools/ (contains bzl build-defs) | ||
---📁 react-native-ios/ | ||
----📁 Libraries/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related but perhaps another topic: I'd like to reconsider how the directories are named/organized in the RN project, which I've found a bit confusing and idiosyncratic for what most see as a "JS project". For example, the exported components and APIs are defined in a variety of places in the repo. (I tried to simplify the situation for contributors to React Native for Web by putting all the public exports in a flat directory)
--📄 {readme, contributing, CoC, other markdown docs} | ||
``` | ||
|
||
I am not fully sure what the layout of the local-cli stuff will end up needing to be after this; with [this proposal](https://github.com/react-native-community/discussions-and-proposals/blob/master/proposals/0002-Extracted-Command-Line-Tools.md) the bulk of it will end up getting moved to another repo anyways. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note: when the cli is moved out it might be a good time to start working towards making this project packager agnostic, and pointing people to working packagers rather than including one in the project. This will surface issues related to the existing packager situation, like OSS RN packages being published without being compiled and being unable to run on pretty much any platform without first being passed through Babel using the RN babel config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OSS RN packages being published without being compiled
IMO I kinda like them shipping un-compiled. Especially with JSI allowing for different JS engines to be used with React Native, it would be useful for it to be easy to use different babel configs for different engines (imo). Biggest thing would be opting in to using native async/await where available for a cleaner error stack trace
Would be worth a discussion of its own for sure!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a difference between not compiling parts of the code that are effectively js standards but not available in older environments, and publishing code with early stage proposals
> - `react-native-ios`: `1.0.0`, `1.0.1`, `1.1.0` | ||
> - `react-native-android`: `1.0.0`, `2.0.0`, `3.0.0` | ||
|
||
This will make React Native's future semver bump to `1.0.0` awkward, if not impossible. Unless it does what React did and jumps to version `60.0.0` or something like that. Or starts at version `4.0.0`, which seems kinda random. NPM also says that for various technical and security reasons, these versions can not be deleted and re-used. I've `npm deprecate`d them, which is the best I can do sadly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively we use package names react-ios
and react-android
(reserved by FB and nice as they drops the "native" part of the name). The former would also open the door to using react-web
and react-windows
(we'd need those transferred to FB) for web/windows-specific modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's definitely an option! That idiom would make it awkward for react-native-dom
however, since react-dom
already exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think react-dom
as a package name makes sense for what it does, and wouldn't make sense as a name for a package in this context. We'd only want a single web implementation, so I don't think we need to worry about mapping every OSS RN variant to an equivalent package in RN.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Def. worth making another issue for, since your idea of having information about out-of-tree platforms in the main repo could probably be done independently of this proposal.
We'd only want a single web implementation
RNWeb and RNDom are different enough projects I think there's room for the both of them. But I really don't know about dropping the native
from the name, because a "React renderer" and a "React Native renderer/platform" are very different things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about going babel’s route and going namespaced.
@react-native/core
@react-native/ios
@react-native/android
@react-native/windows
@react-native/tvos
it would allow for semver 1.0.0 and identify official packages ie react-native-web would immediately be identifiable as an external project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We tried to reach out to the NPM user who owned that namespace, but had no luck in securing it. Would be a good option IMO if we could, however! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry I deleted that message. I was mistaken in my search.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no worries! FWIW, facebook DOES own @react
now. I could see if the owner of @reactnative
would be open to transferring it, but minus the dash -
is a bit weird
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't see who owned @react
because the profile was empty. I reached out to npm asking to be pointed to who has both @react
and @reactnative
earlier, pointing them here.
If @react
is now under facebooks control, if they added both the react and react native community to the org... this would be a great naming convention if everyone got on board:
@react/core
@react/native-core
@react/prop-types
@react/dom
@react/ios
@react/android
@react/web
@react/ etc etc etc
Very straightforward.
EDITED sorry I didn't quote them and tagged instead :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that (and @necolas' suggestion to use react-ios
as well) would probably make sense (especially with version 1 being freed up in them) but would require some more work 👍
I've updated the proposal with both of these formats
|
||
(where `warnMoved` is a hypothetical function warning once that an api is moved to another module) | ||
|
||
And `react-native-ios` would use Haste to import the module from `react-native`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haste is one of those things that is a barrier to OSS contributors; we could probably do this the "standard" way instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that would definitely also make using other packagers (like parcel and webpack) less complex too. @gaearon removed haste from react a while ago and that could be looked to as an example of what could be done:
Haste, however, happens to be really good at doing platform-specific overrides across multiple packages. A good replacement solution for this would need to be devised. Definitely a subject worthy of a proposal/discussion of its own probably!
Not sure if this should be part of this proposal or not, but other people are discussing it, so I'll throw in my two cents. I think we should avoid ever naming modules with the iOS or Android suffix. Even if the other platform doesn't have the necessary APIs available to complete the module's objective it might in the future, causing a rename when it's implemented, or there may be less-common platforms, such as Windows, that do have the necessary APIs that have to implement the awkward module naming for purposes of supporting cross-platform use of the module. I gather the original purpose of doing this was to make it clear that the module only supported particular platforms even for people too lazy to read the docs, but I don't think the possibility of third-party platforms was considered at the time. |
I highly agree that the platform suffix should be dropped from the names! 👍 However in some cases a platform will need a completely different implementation of a component/API and can't be generic. In cases like that it could just go in the platform module, rather than in the main module. |
Right, but hiding implementation details is exactly what React Native is good at, so as long as the behavior isn't wildly unexpected. |
If there are no objections, I will be revising the proposal to drop the platform suffix from the moved APIs |
|
||
## Basic example | ||
|
||
Common APIs and components would remain in `react-native`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How Windows and other platforms would provide their implementations of core components and APIs? Since imports are from react-native
, not from platform-specific module, I guess that would be tricky and require certain platforms to export a minimal subset of APIs to be considered a valid platform for React Native?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Platforms can have an entry like this in their package.json
which adds their platform to the haste search path:
{
"rnpm": {
"haste": {
"providesModuleNodeModules": ["react-native-example"],
"platforms": ["example"]
}
}
}
And if they wanted to override ExampleComponent
in react-native
, react-native-example
would have ExampleComponent.example.js
This solution depends on Haste, so if RN wants to drop Haste in the future a solution for platform overrides will need to be worked out.
I guess that would be tricky and require certain platforms to export a minimal subset of APIs to be considered a valid platform for React Native
Pretty much, or have a whole bunch of stub JS UnimplementedView
files to address the Haste missing files errors
import {AppRegistry, Alert, View} from 'react-native'; | ||
``` | ||
|
||
Platform-specific components and APIs would be imported from the platform's module: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about a module that has 4 methods out of 5 that work on both platforms and wants to provide one, additional, e.g. Alert.promptIOS
? Would the developer need to make an additional Alert
module under react-native-ios
package? Wouldn't that cause troubles for developers to decide what file to use? Should Alert
from react-native-ios
"extend" Alert
from react-native
to keep its methods? Just general questions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think some kinds of revamp of Alert
is very much needed. There's this if
statement in Alert
that basically requires a platform dev to make a platform fork of Alert
. Example from RNWindows:
if (Platform.OS === 'ios') {
if (typeof type !== 'undefined') {
console.warn('Alert.alert() with a 5th "type" parameter is deprecated and will be removed. Use AlertIOS.prompt() instead.');
AlertIOS.alert(title, message, buttons, type);
return;
}
AlertIOS.alert(title, message, buttons);
} else if (Platform.OS === 'android') {
AlertAndroid.alert(title, message, buttons, options);
} else if (Platform.OS === 'windows') {
AlertWindows.alert(title, message, buttons, options);
}
And because it depends on requiring AlertIOS
, it looks like some kind of modification will need to be made to this file in the split.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing a suffix on the method is still bad because just because Android doesn't support it doesn't mean that Windows/MacOS/Ubuntu/other third party platforms won't. Rather than baking it into the method name it should just be documented which platforms it supports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for writing up this proposal! I am really excited to see some updates in terms of re-structuring React Native architecture and enabling more platforms and components to be added on top of its core. Left some comments here and there, will be closely watching this pull request!
|
||
React Native being a Yarn workspace might make it easier to make other modules that should be developed and released alongside the main project. It would be easier to make a more advanced RNTester app in-tree with its own set of dependencies, for example. | ||
|
||
(**UPDATE**: I am working on a pull request to set up a monorepo, so other proposals could benefit from it regardless of this proposal's progress) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just fyi: This can probably not be done on GitHub and will have to be done at FB in one go.
Update: We have split CLI into multiple packages (android, ios and core) and it's been the best decision in a while. I was wondering if there's any outcomes from this PR, such as we're unlikely to implement this architecture or eventually, that's the direction we're going? |
I think the people on our team at FB want these pieces separated as well. However, this is major refactoring work that can only be accomplished internally, so it’s probably a ways out. Probably at least a year until anything could actually happen here unfortunately. We have lots of other projects we have to focus on first. :-) |
Even if it can't happen for a while, it would be good to solidify the plan. |
Is there any updates from Meta team around this ? Thanks! |
What @TheSavior mentioned above still stands:
I don't think we have major updates to share on this side. I believe @rickhanlonii had some opinion on this project and maybe has more to share. Sharing my personal point of view here: I think that reviving this proposal and trying to land it in some form would be highly beneficial. I feel that the current setup of the repo is slowing us down (i.e. it's complicated to export code to separate packages) + it's hard to understand for newcomers (i.e. why iOS code is inside As this RFC looks like a big refactoring, I share all the concerns that were already presented: it should probably be done internally, or at least will require some guidance and coordination from Meta. To make it a bit more actionable, I believe we could split the whole RFC in three big milestones:
Point 3. seems to introduce a lot of complexity due to implications on how we treat APIs that are platform specific, etc. However, I think that 1. and 2. are potentially doable and would be a first step towards unblocking a proper modularization. Curious to hear other's opinion here 👍 |
any news ? |
This is a proposal for a new structure the React Native project could take. I consider it to be part of the spirit of the slimmening, and could also lead to more components and apis being able to be moved out of core.
To clarify: this is not a proposal to move IOS and Android to their own repos. They would be in a monorepo, like React and ReactDOM are
https://github.com/facebook/react/tree/master/packages
I have tried to describe as much as I could, but if there is anything else that should be described in this I would be happy to add to it!
I am happy to champion this effort and this would probably be the biggest open source contribution I've done yet, so it will be quite the learning experience.