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

[React] Depend on npm "react" instead of shipping with a vendored copy #2985

Closed
ide opened this issue Sep 23, 2015 · 60 comments
Closed

[React] Depend on npm "react" instead of shipping with a vendored copy #2985

ide opened this issue Sep 23, 2015 · 60 comments
Assignees
Labels
JavaScript Resolution: Locked This issue was locked by the bot.

Comments

@ide
Copy link
Contributor

ide commented Sep 23, 2015

Pretty certain this is on the radar but want to track it because it's going to make a real difference for the overall React ecosystem when RN can require libraries like react-static-container, react-relay, react-redux, etc... that depend on "react" but not "react-dom".

cc @vjeux @spicyj

@sophiebits
Copy link
Contributor

Yeah, we're blocked on some fbjs stuff where we can't use the open-source copy of fbjs in RN right now because of some Flow conflicts (and maybe packager too?). @gabelevi was going to make a change to Flow that will allow us to move fbjs away from haste so that we can have more than one copy, not sure what the status of that is right now.

We probably won't get everything sorted out in time for React 0.14 but maybe we can do a point release after we fix things up.

cc @sebmarkbage

@skevy
Copy link
Contributor

skevy commented Sep 24, 2015

Yes! This x 1000. Working on a react-router integration with RN right now and I had to dependency inject React into RR. Haha.

Interesting thing though, and something I didn't understand - "react-static-container" actually DOES work on RN. Is it because it requires React, not react, and the packager allows for that?

@sophiebits
Copy link
Contributor

Rest assured that we're aware and making this work was a major motivating factor for the packaging changes.

@skevy
Copy link
Contributor

skevy commented Sep 24, 2015

👍

@ide
Copy link
Contributor Author

ide commented Oct 16, 2015

Redux dropped support for React Native bindings (https://github.com/rackt/react-redux/releases/tag/v4.0.0), and there's also an open issue with Relay compatibility (though Relay has to change a few other things too). So as far as the state of the overall React ecosystem influences your roadmap you might want to give this a little bump in priority.

@sebmarkbage
Copy link
Contributor

It is my top priority after the blocker on fbjs/flow is done (which is unfortunately beyond me atm). cc @zpao

@zpao
Copy link
Member

zpao commented Oct 16, 2015

We're still waiting on flow but as soon as that happens we'll get this sorted.

@ide
Copy link
Contributor Author

ide commented Oct 16, 2015

Nice! Thanks for the update.

@vjeux
Copy link
Contributor

vjeux commented Oct 16, 2015

cc @joesavona

@skevy
Copy link
Contributor

skevy commented Oct 16, 2015

Thanks guys for being so diligent about this. I'm stoked to remove all my "-internal" private npm deps from my projects :)

@rclai
Copy link
Contributor

rclai commented Dec 31, 2015

@sunjay, it means that you will be able to use the latest react-redux NPM package instead of react-redux@3.x and anything that uses react@0.14.0 as a peerDependency.

@gre
Copy link
Contributor

gre commented Jan 1, 2016

I was trying to get this to work on an app that both depends on react@0.14.5 and on react-native@master and I've got:

Error: Naming collision detected:
./node_modules/react/node_modules/fbjs/lib/warning.js
collides with
./node_modules/react-native/node_modules/fbjs/lib/warning.js

however I don't have any node_modules/react-native/node_modules/react

@sophiebits
Copy link
Contributor

@gre Try installing fbjs@0.6.0 at the top level and removing the inner ones? npm 3 would do this for you automatically, I believe.

@gre
Copy link
Contributor

gre commented Jan 1, 2016

I also had to install react-transform-hmr in node_modules/react-native. I have no idea for what reason it wasn't resolved (maybe related to installing from facebook/react-native#master ).

Anyway, thanks it works now :)

@skevy
Copy link
Contributor

skevy commented Jan 2, 2016

@gre the react-transform-hmr dep is just missing from master...that will be cleaned up before the next release.

@gre
Copy link
Contributor

gre commented Jan 2, 2016

@skevy great! I have an unpublished version of gl-react-* libs that work fine with this new version and depending on react & react-native, this is so cool :)

Users will have to not forget about installing fbjs because it doesn't seem to dedup by default. I'm not sure the best way to avoid this, maybe it should be added as a peerDep (maybe not in react-native but at least on each lib that want to be "universal" ?)

@sophiebits
Copy link
Contributor

Sigh. I really wanted to only special-case the one copy of fbjs which is the one that RN uses, so any fbjs in a sibling directory would be okay to be bundled twice (since fbjs modules tend to not be stateful). Guess I'll need to change the packager more to make that work…

@alinz
Copy link

alinz commented Jan 7, 2016

@sunjay I managed to get it to work. You can take a look at the following repo
example-react-native-redux however there are some modification require in package modules which I described here reduxjs/react-redux#236 (comment)

@cancan101
Copy link
Contributor

Any chance this will be fixed for 0.18?

@gre
Copy link
Contributor

gre commented Jan 11, 2016

@cancan101 it will be fixed for 0.18. It is already fixed for 0.18-rc

@cancan101
Copy link
Contributor

Sorry, I should clarify the 'this' i meant is the issue with fbjs and it did not seem to be fixed in the RC.

@corbt
Copy link
Contributor

corbt commented Jan 12, 2016

I ran into the fbjs conflict issue using npm 2. @spicyj's solution of installing fbjs at the root level didn't seem to solve it, but nuking my entire node_modules directory, installing npm 3, and then reinstalling everything did.

The getting started docs should probably be updated to recommend npm 3 if that's the official solution, too bad that it's so slow!

@Mokto
Copy link

Mokto commented Jan 12, 2016

Use @alinz solution, it works ! ;)

@corbt
Copy link
Contributor

corbt commented Jan 12, 2016

@Mokto yeah, but it involves some pretty nasty hacks to work with npm@2 (reaching into packages and deleting the fbjs dependencies).

@alinz
Copy link

alinz commented Jan 12, 2016

@corbt I agree that it involves some folder moving and hack but it should not cause you any problem. fbjs will be moved out from react-native eventually, in a meantime if you have a module that does not work with npm@3 then that would be your only options.

@Mokto
Copy link

Mokto commented Jan 12, 2016

I know it is nasty but you could add this to you package.json :

"postinstall": "npm run setup_project"

@alinz
Copy link

alinz commented Jan 12, 2016

Also we need to detect in setup_project whether user is using npm@2 or npm@3

On Jan 12, 2016, at 3:30 PM, Mokto notifications@github.com wrote:

I know it is nasty but you could add this to you package.json :

"postinstall": "npm run setup_project"


Reply to this email directly or view it on GitHub.

cpojer pushed a commit to facebook/metro that referenced this issue Jan 26, 2017
Summary:
We don't (yet) treat these the same as any other modules because we still have special resolution rules for them in the packager allowing the use of `providesModule`, but I believe this allows people to use npm react in their RN projects and not have duplicate copies of React. Fixes facebook/react-native#2985.

This relies on fbjs 0.6, which includes `.flow` files alongside the `.js` files to allow them to be typechecked without additional configuration. This also uses react 0.14.5, which shims a couple of files (as `.native.js`) to avoid DOM-specific bits. Once we fix these in React, we will use the same code on web and native. Hopefully we can also remove the packager support I'm adding here for `.native.js`.

This diff is not the desired end state for us – ideally the packager would know nothing of react or fbjs, and we'll get there eventually by not relying on `providesModule` in order to load react and fbjs modules. (fbjs change posted here but not merged yet: facebook/fbjs#84.)

This should also allow relay to work seamlessly with RN, but I haven't verified this.

public

Reviewed By: sebmarkbage

Differential Revision: D2786197

fb-gh-sync-id: ff50f28445e949edc9501f4b599df7970813870d
@facebook facebook locked as resolved and limited conversation to collaborators Jul 21, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
JavaScript Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests