Skip to content

Commit 4c84cfb

Browse files
committed
Fix providesModuleNodeModules behavior to be more deterministic.
Currently, the `providesModuleNodeModules` option allows for specifying an array of package names, that the packager will look for within node_modules, no matter how deep they're nested, and treat them as packages that use the `@providesModule` pragma. In reality, this is not actually the behavior we want. npm, because of how it handles dependencies, can do all kinds of arbitrary nesting of packages when `npm install` is run. This causes a problem for how we deal with `providesModuleNodeModules`. Specifically...take `fbjs`. Currently, React Native relies on using the Haste version of `fbjs` (will be resolved in facebook#5084). Thus if npm installs multiple copies of fbjs...which is a very common thing for it to do (as can be seen by this list of issues: https://github.com/facebook/react-native/issues?utf8=%E2%9C%93&q=naming+collision+detected), we get into a state where the packager fails and says that we have a naming collision. Really, the behavior we want is for the packager to treat only a *single* copy of a given package, that we specify in the `Resolver` in the `providesModuleNodeModules` option, as the package that it uses when trying to resolve Haste modules. This PR provides that behavior, by changing `providesModuleNodeModules` from a list of strings to a list of objects that have a `name` key, specifying the package name, as well as a `parent` key. If `parent` is null, it will look for the package at the top level (directly under `node_modules`). If `parent` is specified, it will use the package that is nested under that parent as the Haste module. To anyone who reads this PR and is familiar with the differences between npm2 and npm3 -- this solution works under both, given the fact that we are now shipping the NPM Shrinkwrap file with React Native when it's installed through `npm`. In both the npm2 and npm3 case, node_modules specified by RN's package.json are nested underneath `react-native` in node_modules, thus allowing us to specify, for example, that we want to use React Native's copy of `fbjs` (not any other copy that may be installed) as the module used by the packager to resolve any `requires` that reference a module in `fbjs`.
1 parent f2b95f2 commit 4c84cfb

File tree

3 files changed

+52
-13
lines changed

3 files changed

+52
-13
lines changed

packager/react-packager/src/DependencyResolver/DependencyGraph/DependencyGraphHelpers.js

Lines changed: 38 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
/**
1+
/**
22
* Copyright (c) 2015-present, Facebook, Inc.
33
* All rights reserved.
44
*
@@ -16,16 +16,48 @@ class DependencyGraphHelpers {
1616
this._assetExts = assetExts;
1717
}
1818

19+
/**
20+
* This method has three possible outcomes:
21+
*
22+
* 1) A file is not in "node_modules" at all (some type of file in the project).
23+
* 2) The file is in "node_modules", and it's contained in one of the
24+
* "providesModuleNodeModules" packages.
25+
* 3) It's in "node_modules" but not in a "providesModuleNodeModule" package,
26+
* so it's just a normal node_module.
27+
*
28+
* An item in the `providesModuleNodeModule` is an object containing two keys:
29+
* * "name" - the package name
30+
* * "parent" - if a string, it's the name of the parent package of the module in
31+
* the dep tree. If null, it signifies that we should look for this package at
32+
* the top level of the module tree.
33+
*/
1934
isNodeModulesDir(file) {
20-
const index = file.lastIndexOf('/node_modules/');
35+
const index = file.indexOf('/node_modules/');
2136
if (index === -1) {
2237
return false;
2338
}
2439

25-
const parts = file.substr(index + 14).split(path.sep);
26-
const dirs = this._providesModuleNodeModules;
27-
for (let i = 0; i < dirs.length; i++) {
28-
if (parts.indexOf(dirs[i]) > -1) {
40+
const lastIndex = file.lastIndexOf('/node_modules/');
41+
const fullPathParts = file.substr(index + 14).split(path.sep);
42+
const packageParts = file.substr(lastIndex + 14).split(path.sep);
43+
const packageName = packageParts[0];
44+
45+
const modules = this._providesModuleNodeModules;
46+
for (let i = 0; i < modules.length; i++) {
47+
const providesModuleNodeModule = modules[i];
48+
const providesModulePackageName = providesModuleNodeModule.name;
49+
const providesModuleParent = providesModuleNodeModule.parent;
50+
51+
let parent = null;
52+
// if the top level package name is not the same as what we already
53+
// computed the "packageName" to be, then make sure we record that
54+
// that package is the parent
55+
if (fullPathParts[0] !== packageName) {
56+
parent = fullPathParts[0];
57+
}
58+
59+
if (providesModulePackageName === packageName &&
60+
providesModuleParent === parent) {
2961
return false;
3062
}
3163
}

packager/react-packager/src/DependencyResolver/DependencyGraph/__tests__/DependencyGraph-test.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -90,9 +90,9 @@ describe('DependencyGraph', function() {
9090
cache: new Cache(),
9191
fileWatcher,
9292
providesModuleNodeModules: [
93-
'haste-fbjs',
94-
'react-haste',
95-
'react-native',
93+
{ name: 'haste-fbjs', parent: null },
94+
{ name: 'react-haste', parent: null },
95+
{ name: 'react-native', parent: null },
9696
// Parse requires AsyncStorage. They will
9797
// change that to require('react-native') which
9898
// should work after this release and we can

packager/react-packager/src/Resolver/index.js

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -85,14 +85,21 @@ class Resolver {
8585
(opts.blacklistRE && opts.blacklistRE.test(filepath));
8686
},
8787
providesModuleNodeModules: [
88-
'fbjs',
89-
'react',
90-
'react-native',
88+
// Use the project's installed version of react-native
89+
// as the "haste" version of `react-native`
90+
// (and ignore any nested copies);
91+
{ name: 'react-native', parent: null },
92+
// Use the react peerDep for the "haste"
93+
// version of `react`.
94+
{ name: 'react', parent: null },
95+
// We only want to use the fbjs underneath react-native
96+
// as the "haste" version of `fbjs`.
97+
{ name: 'fbjs', parent: 'react-native'},
9198
// Parse requires AsyncStorage. They will
9299
// change that to require('react-native') which
93100
// should work after this release and we can
94101
// remove it from here.
95-
'parse',
102+
{ name: 'parse', parent: null },
96103
],
97104
platforms: ['ios', 'android'],
98105
preferNativePlatform: true,

0 commit comments

Comments
 (0)