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

The Slimmening #6

Open
kelset opened this Issue Aug 1, 2018 · 23 comments

Comments

Projects
None yet
@kelset
Collaborator

kelset commented Aug 1, 2018

Intro

With this issue I'd like to try and create a "one stop" for all the information available around the discussion that has been going on for a long time within the Core team about this project - and explain the label.

The Gist of it

React Native is currently a huge repo. Would it make sense to move UI components (ScrollView, Switches, WebView) and Native Modules like PushNotifications, etc into a separate repos?

The basic answer is yes, and in the past few months this has started becoming a reality via a few proposals and some commits from the FB team.

You can check the full list of native components that are being considered for extraction/deprecation here, but proposals can also be about JS-only components.

Advantages

  • Chance to deprecate older modules
  • Make the codebase more approachable
  • Reduce the Bundle size for projects that don't use the "extractable" components, which would lead to faster startup times for the apps
  • Help the community move fast and enable pull requests to be reviewed and merged quicker

Doubts (with "answers" to discuss about)

A) Should we define a roadmap that each "separation" will have to follow?

The rough rule, for now, is to follow this set of steps:

  • version N: the proposal gets merged, new repo gets created and changelog communicates it
  • version N+1: the component, if used by the main repo, triggers a Warning about deprecation
  • version N+2: the code is fully removed and developers will need to rely on the new repo for it.

For "pure removal" of old code, this may happen in a "2 version" window instead (ex. the old NavigatorIOS, removal announced in 57 and will happen in 58).

This is still a WIP so feedback on how to make it better is appreciated. Ideally we'd have a dedicated document with the details on how the flow of an extraction works, so that everyone on the community can understand all the steps involved and can take ownership of a proposal for an extraction.

B) How would this affect platforms like React-native-windows?

Owners of those out-of-tree platforms should be involved in the conversations to ensure a smooth transition.

C) Who would retain ownership (and "responsibility to maintain") the separated component?

For "extractions", the new repositories will live under the react-native-community umbrella. Details around npm deployment are still being discussed, expect a dedicated issue about that.

For "deprecations/removals" they will be probably be just removed from the main repo, and eventually "ported" to this repository for deprecated-modules by FB by developers that still need it.


EDIT: this has been massively edited to provide more informations about the slimmening and reflect the ongoing discussion. And since things are still moving, please keep an eye on this post because it will evolve more.

@eliperkins

This comment has been minimized.

Show comment
Hide comment
@eliperkins

eliperkins Aug 1, 2018

Make the codebase more approachable

I don't know that this will necessarily be the case. If code ends up being spread throughout many modules and repos, it's going to be hard to find exactly the code we're looking for. Having the React Native renderer in the React repo is already an example of this. It's trivial to search the repo now for the specific component you're looking for and find all the code related to it.

Reduce the Bundle size for projects that don't use the "extractable" components, which would lead to faster startup times for the apps

I believe we can accomplish this in ways that don't include separating code in the repo.

Have we explored using Lerna or Yarn workspaces to help alleviate some of this?

eliperkins commented Aug 1, 2018

Make the codebase more approachable

I don't know that this will necessarily be the case. If code ends up being spread throughout many modules and repos, it's going to be hard to find exactly the code we're looking for. Having the React Native renderer in the React repo is already an example of this. It's trivial to search the repo now for the specific component you're looking for and find all the code related to it.

Reduce the Bundle size for projects that don't use the "extractable" components, which would lead to faster startup times for the apps

I believe we can accomplish this in ways that don't include separating code in the repo.

Have we explored using Lerna or Yarn workspaces to help alleviate some of this?

@kelset

This comment has been minimized.

Show comment
Hide comment
@kelset

kelset Aug 1, 2018

Collaborator

👋 @eliperkins I see you points, just want to underline how this is a temp post while the proposal is being written up :)

You should discuss those points there once it's up (which should be soon, @axe-fb is writing it AFAIK).

Collaborator

kelset commented Aug 1, 2018

👋 @eliperkins I see you points, just want to underline how this is a temp post while the proposal is being written up :)

You should discuss those points there once it's up (which should be soon, @axe-fb is writing it AFAIK).

@satya164

This comment has been minimized.

Show comment
Hide comment
@satya164

satya164 Aug 1, 2018

Member

I think components like ScrollView which are absolutely necessary when building any app should remain in the core. Separating components such as WebView, ViewPager, DrawerLayoutAndroid etc. make sense.

Reduce the Bundle size for projects that don't use the "extractable" components, which would lead to faster startup times for the apps

Metro should support tree-shaking. Then RN can use ES exports which will reduce the bundle size for JavaScript code that's not used.

For native code, on iOS, devs have to include the modules they are going to use, not sure why this isn't the case on Android. But it would help to reduce the native code size.

Member

satya164 commented Aug 1, 2018

I think components like ScrollView which are absolutely necessary when building any app should remain in the core. Separating components such as WebView, ViewPager, DrawerLayoutAndroid etc. make sense.

Reduce the Bundle size for projects that don't use the "extractable" components, which would lead to faster startup times for the apps

Metro should support tree-shaking. Then RN can use ES exports which will reduce the bundle size for JavaScript code that's not used.

For native code, on iOS, devs have to include the modules they are going to use, not sure why this isn't the case on Android. But it would help to reduce the native code size.

@axe-fb

This comment has been minimized.

Show comment
Hide comment
@axe-fb

axe-fb Aug 6, 2018

Collaborator

@kelset - I started working on a proposal, but I think that each Native Module and View Manager may be slightly different. Instead of writing one blanket proposal for all components, I think it will be more actionable to write up proposal for sets of components.

I am thinking that the work itself could be done in two phases. Here is a set of Native Modules that I have looked at.

Phase 1

Remove all components that we think can be deprecated. This is the rows in yellow.

Phase 2

Remove maintained components out of the repo.

Some of the other reasons I think we should do this

  1. Make upgrading easier - today, a breaking change in ONE component prevents teams from upgrading entire React Native. With massive changes like Fabric, we need the ability to be able to continue upgrading React-Native core, and allowing gradual upgrades of things like native modules.

  2. Faster PRs and responding to issues. Though we tag issues and PRs, at this point it seems a lot. Also, moving components outside will also enable other contributors merge in PRs faster. Given the surface of React Native, we do need more help, and need to add to core contributors.

Collaborator

axe-fb commented Aug 6, 2018

@kelset - I started working on a proposal, but I think that each Native Module and View Manager may be slightly different. Instead of writing one blanket proposal for all components, I think it will be more actionable to write up proposal for sets of components.

I am thinking that the work itself could be done in two phases. Here is a set of Native Modules that I have looked at.

Phase 1

Remove all components that we think can be deprecated. This is the rows in yellow.

Phase 2

Remove maintained components out of the repo.

Some of the other reasons I think we should do this

  1. Make upgrading easier - today, a breaking change in ONE component prevents teams from upgrading entire React Native. With massive changes like Fabric, we need the ability to be able to continue upgrading React-Native core, and allowing gradual upgrades of things like native modules.

  2. Faster PRs and responding to issues. Though we tag issues and PRs, at this point it seems a lot. Also, moving components outside will also enable other contributors merge in PRs faster. Given the surface of React Native, we do need more help, and need to add to core contributors.

@axe-fb

This comment has been minimized.

Show comment
Hide comment
@axe-fb

axe-fb Aug 6, 2018

Collaborator

@eliperkins - Thanks for taking a look at the idea.
I did look into Lerna workspaces. While it is potentially useful for typical cases of testing, I am not sure how it would really benefit during development. As I understand, most people typically work on a single component at a time, and having the ability to have a simple RN app with just that component may make it more manageable, and will also be easier to get started with.

It's trivial to search the repo now for the specific component you're looking for and find all the code related to it.

I would like to believe that in most cases, the code of a component or native module should live at one single place. Today, JS and Native code is separated, which makes it harder. But putting them all in one repo, it would be much easier to develop/debug.

Having the React Native renderer in the React repo is already an example of this.

I think you are right. We would like to eventually like to move to a world where React is the top level repo, and REact-dom is a renderer, just like React-native, or React-VR. I think that by separating core from components and native modules would help with that.

Bundle Size

Also be making core components like ScrollView or Flatlist at the same level as other components, we can ensure that people who don't use core components remove them like regular components.

If you think these explanations don't make sense, please let me know - I can try to elaborate !!

Collaborator

axe-fb commented Aug 6, 2018

@eliperkins - Thanks for taking a look at the idea.
I did look into Lerna workspaces. While it is potentially useful for typical cases of testing, I am not sure how it would really benefit during development. As I understand, most people typically work on a single component at a time, and having the ability to have a simple RN app with just that component may make it more manageable, and will also be easier to get started with.

It's trivial to search the repo now for the specific component you're looking for and find all the code related to it.

I would like to believe that in most cases, the code of a component or native module should live at one single place. Today, JS and Native code is separated, which makes it harder. But putting them all in one repo, it would be much easier to develop/debug.

Having the React Native renderer in the React repo is already an example of this.

I think you are right. We would like to eventually like to move to a world where React is the top level repo, and REact-dom is a renderer, just like React-native, or React-VR. I think that by separating core from components and native modules would help with that.

Bundle Size

Also be making core components like ScrollView or Flatlist at the same level as other components, we can ensure that people who don't use core components remove them like regular components.

If you think these explanations don't make sense, please let me know - I can try to elaborate !!

@axe-fb

This comment has been minimized.

Show comment
Hide comment
@axe-fb

axe-fb Aug 6, 2018

Collaborator

@satya164
Could you please explain "tree-shaking" ?
Could you also please explain the point about "For native code, on iOS, devs have to include the modules they are going to use, not sure why this isn't the case on Android. But it would help to reduce the native code size" ?

Collaborator

axe-fb commented Aug 6, 2018

@satya164
Could you please explain "tree-shaking" ?
Could you also please explain the point about "For native code, on iOS, devs have to include the modules they are going to use, not sure why this isn't the case on Android. But it would help to reduce the native code size" ?

@satya164

This comment has been minimized.

Show comment
Hide comment
@satya164

satya164 Aug 6, 2018

Member

@axe-fb

Tree-shaking is when the bundler marks unused imports as dead code which is then removed by the uglifier. Since ES imports are statically analyzable, it's easy to detect which modules are unused and mark them as dead code. All the major bundlers on the web support it, such as Webpack, Rollup, and Parcel. Rollup and Parcel also support tree-shaking on CommonJS modules. Metro doesn't support this feature.

So when you start a React Native iOS project, not all of the native modules and components are linked by default. If you want to use a native module that's not linked by default, you need to link it manually (for example the blob module on iOS is not linked by default). Whereas on Android all of the available native modules and components are already linked by default.

Member

satya164 commented Aug 6, 2018

@axe-fb

Tree-shaking is when the bundler marks unused imports as dead code which is then removed by the uglifier. Since ES imports are statically analyzable, it's easy to detect which modules are unused and mark them as dead code. All the major bundlers on the web support it, such as Webpack, Rollup, and Parcel. Rollup and Parcel also support tree-shaking on CommonJS modules. Metro doesn't support this feature.

So when you start a React Native iOS project, not all of the native modules and components are linked by default. If you want to use a native module that's not linked by default, you need to link it manually (for example the blob module on iOS is not linked by default). Whereas on Android all of the available native modules and components are already linked by default.

@jamonholmgren

This comment has been minimized.

Show comment
Hide comment
@jamonholmgren

jamonholmgren Aug 6, 2018

Collaborator

We used Lerna on a project (https://github.com/infinitered/gluegun) and, while it's a very cool system, I found it wasn't really worth the extra confusion it generated among new contributors.

Collaborator

jamonholmgren commented Aug 6, 2018

We used Lerna on a project (https://github.com/infinitered/gluegun) and, while it's a very cool system, I found it wasn't really worth the extra confusion it generated among new contributors.

@gwmccull

This comment has been minimized.

Show comment
Hide comment
@gwmccull

gwmccull Aug 9, 2018

I remember a year ago or so, someone came up with a list of the components that every renderer needed to support if they were going to be able to able to render an app. It was things like View and Text. As I recall, there were around 7 components and a handful of APIs plus the bridge. I think the list may have been put together by the person who built react-native-xp but my google-fu is weak today so I can't find it

All of that to say, it seems like the React Native repo should have the minimal list of components/APIs/code required for every app and then everything else should be in a separate repo

gwmccull commented Aug 9, 2018

I remember a year ago or so, someone came up with a list of the components that every renderer needed to support if they were going to be able to able to render an app. It was things like View and Text. As I recall, there were around 7 components and a handful of APIs plus the bridge. I think the list may have been put together by the person who built react-native-xp but my google-fu is weak today so I can't find it

All of that to say, it seems like the React Native repo should have the minimal list of components/APIs/code required for every app and then everything else should be in a separate repo

@TheSavior

This comment has been minimized.

Show comment
Hide comment
@TheSavior

TheSavior Sep 6, 2018

At React Native EU today there was a conversation with some of the core contributors around why we haven't removed NavigatorIOS yet. The answer was that it is part of the slimmening effort but just nobody had done it yet.

@fkgozali just put up an internal commit removing it from React Native. That will be landing shortly.

TheSavior commented Sep 6, 2018

At React Native EU today there was a conversation with some of the core contributors around why we haven't removed NavigatorIOS yet. The answer was that it is part of the slimmening effort but just nobody had done it yet.

@fkgozali just put up an internal commit removing it from React Native. That will be landing shortly.

facebook-github-bot added a commit to facebook/react-native that referenced this issue Sep 7, 2018

Remove NavigatorIOS
Summary:
Legacy navigator impl. There are other alternatives that should be used instead.

Part of the slimmening effort as described here: react-native-community/discussions-and-proposals#6

Reviewed By: TheSavior

Differential Revision: D9677824

fbshipit-source-id: 24ae500751d2a8c398f246d36604a58f0b3c113b
@fkgozali

This comment has been minimized.

Show comment
Hide comment
@fkgozali

fkgozali Sep 7, 2018

put up an internal commit removing it from React Native. That will be landing shortly.

here’s the commit: facebook/react-native@0df92af

fkgozali commented Sep 7, 2018

put up an internal commit removing it from React Native. That will be landing shortly.

here’s the commit: facebook/react-native@0df92af

@turnrye

This comment has been minimized.

Show comment
Hide comment
@turnrye

turnrye Sep 9, 2018

Member

Expect a properly written PR with the proposal for it, this is a "temporary" post for transparency purposes.

@kelset is this still planned?

Member

turnrye commented Sep 9, 2018

Expect a properly written PR with the proposal for it, this is a "temporary" post for transparency purposes.

@kelset is this still planned?

@kelset

This comment has been minimized.

Show comment
Hide comment
@kelset

kelset Sep 10, 2018

Collaborator

No, actually 😅

I'm planning on reworking the top post a bit (based on a couple things discussed during RNEU) but haven't had time yet 🙈 Will do it this week

Collaborator

kelset commented Sep 10, 2018

No, actually 😅

I'm planning on reworking the top post a bit (based on a couple things discussed during RNEU) but haven't had time yet 🙈 Will do it this week

@axe-fb

This comment has been minimized.

Show comment
Hide comment
@axe-fb

axe-fb Sep 10, 2018

Collaborator

@turnrye - I think we don't need a top level PR. We could have PRs for individual components that we want to remove and move out - like what we did for webview.

Collaborator

axe-fb commented Sep 10, 2018

@turnrye - I think we don't need a top level PR. We could have PRs for individual components that we want to remove and move out - like what we did for webview.

@kelset

This comment has been minimized.

Show comment
Hide comment
@kelset

kelset Sep 12, 2018

Collaborator

I've updated the top post, let me know / let's discuss about how to make the slimmening better.

Collaborator

kelset commented Sep 12, 2018

I've updated the top post, let me know / let's discuss about how to make the slimmening better.

@grabbou

This comment has been minimized.

Show comment
Hide comment
@grabbou

grabbou Sep 12, 2018

Contributor

Thank you for updating the original issue @kelset with some recent changes we have discussed together.

Generally speaking, I really like the direction we are moving towards. I have one comment on the roadmap for deprecating and eventually, removing modules from the core.

As a part of the first step (listed below):

version N: the proposal gets merged, new repo gets created and changelog communicates it

I would also think about removing the module that is the subject of a particular extraction and requiring it from the new module right away. That way, we would automatically opt-in everyone for the latest version of a particular module and make it easy to verify that the newly created API and established release process works without issues.

I would imagine we could print a deprecation message along the following lines:

WebView has been moved away from React Native core to react-native-web-view package. This version of React Native required that package for you behind the scenes. In the next version, you will have to change your imports to access WebView from that package instead.

Contributor

grabbou commented Sep 12, 2018

Thank you for updating the original issue @kelset with some recent changes we have discussed together.

Generally speaking, I really like the direction we are moving towards. I have one comment on the roadmap for deprecating and eventually, removing modules from the core.

As a part of the first step (listed below):

version N: the proposal gets merged, new repo gets created and changelog communicates it

I would also think about removing the module that is the subject of a particular extraction and requiring it from the new module right away. That way, we would automatically opt-in everyone for the latest version of a particular module and make it easy to verify that the newly created API and established release process works without issues.

I would imagine we could print a deprecation message along the following lines:

WebView has been moved away from React Native core to react-native-web-view package. This version of React Native required that package for you behind the scenes. In the next version, you will have to change your imports to access WebView from that package instead.

@linjson

This comment has been minimized.

Show comment
Hide comment
@linjson

linjson Sep 14, 2018

@kelset ,NavigatorIOS will be removed in rn-0.58,would you put it in facebookarchive/react-native-custom-components

linjson commented Sep 14, 2018

@kelset ,NavigatorIOS will be removed in rn-0.58,would you put it in facebookarchive/react-native-custom-components

gengjiawen added a commit to gengjiawen/react-native that referenced this issue Sep 14, 2018

Remove NavigatorIOS
Summary:
Legacy navigator impl. There are other alternatives that should be used instead.

Part of the slimmening effort as described here: react-native-community/discussions-and-proposals#6

Reviewed By: TheSavior

Differential Revision: D9677824

fbshipit-source-id: 24ae500751d2a8c398f246d36604a58f0b3c113b
@kelset

This comment has been minimized.

Show comment
Hide comment
@kelset

kelset Sep 17, 2018

Collaborator

@linjson I suggest you do it yourself if you still need that component. Or maintain your own version of it.

Collaborator

kelset commented Sep 17, 2018

@linjson I suggest you do it yourself if you still need that component. Or maintain your own version of it.

@ricardobeat

This comment has been minimized.

Show comment
Hide comment
@ricardobeat

ricardobeat Sep 18, 2018

Does anybody have a rough estimate for impact on bundle size & startup time when removing NavigatorIOS + WebView?

ricardobeat commented Sep 18, 2018

Does anybody have a rough estimate for impact on bundle size & startup time when removing NavigatorIOS + WebView?

@karanjthakkar

This comment has been minimized.

Show comment
Hide comment
@karanjthakkar

karanjthakkar Sep 18, 2018

@ricardobeat This tweet by @fkgozali suggests that with NavigatorIOS removed, the savings should be 20kb+. I guess @jamonholmgren would have more info on the webview savings once he has extracted it from core and deleted the necessary code.

karanjthakkar commented Sep 18, 2018

@ricardobeat This tweet by @fkgozali suggests that with NavigatorIOS removed, the savings should be 20kb+. I guess @jamonholmgren would have more info on the webview savings once he has extracted it from core and deleted the necessary code.

@kelset

This comment has been minimized.

Show comment
Hide comment
@kelset

kelset Sep 18, 2018

Collaborator

That said, since those two extractions will happen as separate times (the full extraction of WebView won't happen in 0.58) I don't think it's too relevant to get a combined "benchmark".

Collaborator

kelset commented Sep 18, 2018

That said, since those two extractions will happen as separate times (the full extraction of WebView won't happen in 0.58) I don't think it's too relevant to get a combined "benchmark".

@jamonholmgren

This comment has been minimized.

Show comment
Hide comment
@jamonholmgren

jamonholmgren Sep 18, 2018

Collaborator

I don't currently have an estimate on savings, but that would be interesting to know.

Collaborator

jamonholmgren commented Sep 18, 2018

I don't currently have an estimate on savings, but that would be interesting to know.

@swashcap

This comment has been minimized.

Show comment
Hide comment
@swashcap

swashcap Sep 18, 2018

There's two problems:

  • React Native has components for some old/deprecated native components that should be removed. Maintenance hasn't been a priority, so these components hang around for a long time. This will continue to be a problem as native UI frameworks change.
  • React Native packages stuff that many applications don't use (both native + JS code).

Does moving away from the current monorepo solve these problems?

Anecdotally, I've seen separation into many repositories come with problems: it's significantly difficult to perform integration tests, and a non-trivial amount of time is spent assuring versions of packages align -- even with decent tooling. Versioning then becomes a developer concern (although, I guess this is normal in JS projects). Then there's the problem of discoverability: where do folks find the de facto WebView? Can developers easily find the right component?

Just some initial thoughts. Glad to hear this stuff’s being carefully considered.

swashcap commented Sep 18, 2018

There's two problems:

  • React Native has components for some old/deprecated native components that should be removed. Maintenance hasn't been a priority, so these components hang around for a long time. This will continue to be a problem as native UI frameworks change.
  • React Native packages stuff that many applications don't use (both native + JS code).

Does moving away from the current monorepo solve these problems?

Anecdotally, I've seen separation into many repositories come with problems: it's significantly difficult to perform integration tests, and a non-trivial amount of time is spent assuring versions of packages align -- even with decent tooling. Versioning then becomes a developer concern (although, I guess this is normal in JS projects). Then there's the problem of discoverability: where do folks find the de facto WebView? Can developers easily find the right component?

Just some initial thoughts. Glad to hear this stuff’s being carefully considered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment