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

Require modules from React Native as node modules. #6715

Merged

Conversation

davidaurelio
Copy link
Contributor

This is one puzzle piece to remove the special handling of node_modules/react in React Native’s packager.

It changes all requires into React Native from @providesModule to the node.js resolution algorithm.

I tested our internal use case by building the v15.0.3-dev branch with this patch applied and running an app.

RCTEventEmitter: 'react-native/Libraries/BatchedBridge/BatchedBridgedModules/RCTEventEmitter',
TextInputState: 'react-native/Libraries/Components/TextInput/TextInputState',
UIManager: 'react-native/Libraries/Utilities/UIManager',
UIManagerStatTracker: 'react-native/Libraries/ReactNative/UIManagerStatTracker',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one is commented out in ReactNativeBaseComponent.js

@sebmarkbage
Copy link
Collaborator

This makes it very tough to change internal structures of React Native. Have you considered a build script that puts them into the same repo like we do for React? Or maybe putting forwarding modules into a common directory? That seems to be a pretty common pattern for solving this problem with C headers.

@sebmarkbage
Copy link
Collaborator

Hm. This only works if react and react-native happens to be at the same level of node_modules hierarchies, right?

I can imagine scenarios where this is not the case. E.g. when you use an application framework as a dependency which uses react-native to render so that's an indirect dependency but react is a peerDependency so it ends up at the top level.

I think the problem is that we can't make react-native a dependency of react to clarify this to npm.

Even with npm3 there are scenarios where you end up with nested dependencies, right?

@sebmarkbage
Copy link
Collaborator

I kind of think that some of these should go on an indirection on the global object directly instead of in the module system since there is no way in the node/npm world to define a global host environment module other than through the global object. E.g. global.process.UIManager

@satya164
Copy link

satya164 commented May 7, 2016

Can't we use require.resolve instead of hardcoding the path?

@davidaurelio
Copy link
Contributor Author

@sebmarkbage thank you for the feedback

This makes it very tough to change internal structures of React Native.

You are correct here. I chose this variant to maintain backwards compatibility. If react@15.0.3 is not compatible with older versions of react native anyway, I will introduce the necessary forwarding modules and change this PR.

Have you considered a build script that puts them into the same repo like we do for React? Or maybe putting forwarding modules into a common directory? That seems to be a pretty common pattern for solving this problem with C headers.

Yes, the build script will come soon. We will need a separate solution for our internal setup, but as long as the forwarding modules are in the same place there won’t be a problem.

Hm. This only works if react and react-native happens to be at the same level of node_modules hierarchies, right?

This will work if react is installed alongside or as dependency of react-native.

E.g. when you use an application framework as a dependency which uses react-native to render so that's an indirect dependency but react is a peerDependency so it ends up at the top level.

That’s correct. Does such a framework exist yet? If not, we’re good and can encourage to use react-native as a peer dependency, just like react-native uses react as a peer dependency. (And that means that in your hypothetical scenario we’d end up with two copies of react).

I think the problem is that we can't make react-native a dependency of react to clarify this to npm.
Even with npm3 there are scenarios where you end up with nested dependencies, right?

Both assumptions are correct, but react-native specifying react as a peer dependency pretty much means that we require them to be installed side-by-side.

I kind of think that some of these should go on an indirection on the global object directly instead of in the module system since there is no way in the node/npm world to define a global host environment module other than through the global object. E.g. global.process.UIManager

That would certainly be a possibility, given that we have many stateful global modules anyway. There are a lot of opinions on this topic, and if we’d go that route I’d like to keep that a separate step from this PR.

I will add forwarding modules to RN tomorrow and update this PR accordingly.

@zpao
Copy link
Member

zpao commented May 9, 2016

You are correct here. I chose this variant to maintain backwards compatibility. If react@15.0.3 is not compatible with older versions of react native anyway, I will introduce the necessary forwarding modules and change this PR.

FWIW, I'm pretty sure that regardless of how this gets done it's not going into 15.0.3 and we'd do it as a 15.y.0.

@davidaurelio
Copy link
Contributor Author

@zpao the version does not matter to me. I am more interested if the recent reorganization is backwards-compatible or not. I guess it isn’t, because we would end up with having the same @providesModule files both in react and react-native and get a conflict error from RN packager.

@davidaurelio davidaurelio force-pushed the react-native-imports-as-node_modules branch 2 times, most recently from 211c010 to 05319ff Compare May 10, 2016 15:15
@ghost
Copy link

ghost commented May 10, 2016

@davidaurelio updated the pull request.

@davidaurelio
Copy link
Contributor Author

counterpart is D3281736

@davidaurelio
Copy link
Contributor Author

friendly ping

whiteListNames.forEach(function(name) {
moduleMap[name] = name;
Object.keys(whiteList).forEach(function(name) {
moduleMap[name] = whiteList[name];
});

moduleMap['object-assign'] = 'object-assign';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just simplify this whole thing since you're changing to an object anyway?

var moduleMap = Object.assign(
  {},
  require('fbjs/module-map'),
  {
    deepDiffer: 'react-native/lib/deepDiffer',
    ...
    'object-assign': 'object-assign',
  }
)

Otherwise, this seems fine. If you say the module resolution works then I believe you :) I'm a little concerned about the framework & peerDep and that screwing up the resolution, but we can tackle that later. I think the only clear solution to that is shipping the renderer in its own package, which we'll get to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, I can change that. Having the renderer in its own package makes a lot of sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. We'll land after you update and ship 15.1.0-alpha.2 so RN can pull this in officially

@davidaurelio davidaurelio force-pushed the react-native-imports-as-node_modules branch from 05319ff to 115c137 Compare May 13, 2016 22:52
@davidaurelio davidaurelio force-pushed the react-native-imports-as-node_modules branch from 115c137 to d1fc37b Compare May 13, 2016 22:58
@ghost
Copy link

ghost commented May 13, 2016

@davidaurelio updated the pull request.

@zpao zpao merged commit 151e1d7 into facebook:master May 16, 2016
@zpao zpao added this to the 15.y.0 milestone May 16, 2016
@zpao zpao removed this from the 15.y.0 milestone Jun 1, 2016
@zpao zpao modified the milestones: 15-next, 15.y.0 Jun 1, 2016
zpao pushed a commit to zpao/react that referenced this pull request Jun 8, 2016
zpao pushed a commit that referenced this pull request Jun 14, 2016
@zpao zpao modified the milestones: 15-next, 15.2.0 Jun 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants