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

Move iOS and Android Specific Code to Their Own Packages #49

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

empyrical
Copy link
Member

@empyrical empyrical commented Nov 2, 2018

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.

Copy link

@elicwhite elicwhite left a 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:

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?

Copy link
Member Author

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

Copy link
Collaborator

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.

Copy link
Member Author

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.

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.

Copy link
Member Author

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`

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.

Copy link
Member Author

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 👍

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.

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?

Copy link
Member Author

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.

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.

@empyrical
Copy link
Member Author

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.

Copy link
Member

@sibelius sibelius left a 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

@kelset kelset added the 💡 Proposal This label identifies a proposal label Nov 2, 2018
@empyrical
Copy link
Member Author

Do you mean, like, moving all platform-specific components into userland instead of splitting them out into platform modules?

Copy link

@necolas necolas left a 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`
Copy link

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?

Copy link
Member Author

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`
Copy link

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`
Copy link

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`
Copy link

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`
Copy link

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`
Copy link

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/
Copy link

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.
Copy link

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.

Copy link
Member Author

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!

Copy link

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.
Copy link

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.

Copy link
Member Author

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.

Copy link

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.

Copy link
Member Author

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.

Copy link

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.

Copy link
Member Author

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! 👍

Copy link

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.

Copy link
Member Author

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

Copy link

@deavial deavial Nov 8, 2018

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

Copy link
Member Author

@empyrical empyrical Nov 8, 2018

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`:
Copy link

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.

Copy link
Member Author

@empyrical empyrical Nov 2, 2018

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:

facebook/react#11303

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!

@kevinvangelder
Copy link

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.

@empyrical
Copy link
Member Author

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. DatePicker... is a good example of this. It's a component on IOS, but an API that summons a dialog on Android.

@kevinvangelder
Copy link

Right, but hiding implementation details is exactly what React Native is good at, so as long as the behavior isn't wildly unexpected.

@empyrical
Copy link
Member Author

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`:
Copy link
Member

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?

Copy link
Member Author

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:
Copy link
Member

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

Copy link
Member Author

@empyrical empyrical Nov 4, 2018

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.

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.

Copy link
Member

@grabbou grabbou left a 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)
Copy link
Member

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.

@grabbou
Copy link
Member

grabbou commented Feb 13, 2020

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?

@elicwhite
Copy link

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. :-)

@kevinvangelder
Copy link

Even if it can't happen for a while, it would be good to solidify the plan.

@larakitb
Copy link

larakitb commented Feb 13, 2022

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. :-)

Is there any updates from Meta team around this ? Thanks!

@cortinico
Copy link
Member

Is there any updates from Meta team around this ? Thanks!

What @TheSavior mentioned above still stands:

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. :-)

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 React and Android code inside ReactAndroid?).

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:

  1. Re-organize the project into a proper Yarn workspace - Essentially, having react-native as a top level package and having repo-config/package.json as our workspace is really suboptimal. It introduces a lot of complexity and makes it hard to extract anything inside the package/ folder we currently have.

  2. Align versioning of react-native and the other packages we currently have, and let the CI publish them - Currently the versions of react-native we have on main is updated from 1000.0.0 to the needed one whenever we publish a nightly/release/release-candidate. That's not true for the other NPM packages inside packages/ folder. Those packages are following arbitrary version schemas and are published on a "per-need" basis. This means that our nightly versions of RN are not actually using all the code we have on main. Only the react-native package is doing so, other dependencies (i.e. react-native-codegen) are not updated and we might have code on main for them, which is not actually published. I think that we should align the versioning of all the packages to the RN one. Moreover, we should potentially move to having the CI publish all the packages, not only react-native.

  3. Modularize react-native. I.e. Split the Android/iOS code into separate packages (react-native-ios, react-native-android and more).

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 👍

@vksgautam1
Copy link

any news ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 Proposal This label identifies a proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.