Skip to content
This repository was archived by the owner on Jan 13, 2022. It is now read-only.

Commit 6e092bb

Browse files
skevyskevy
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 #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 3858dc7 commit 6e092bb

File tree

4 files changed

+332
-17
lines changed

4 files changed

+332
-17
lines changed

‎src/DependencyGraph/DependencyGraphHelpers.js

Lines changed: 103 additions & 13 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
*
@@ -8,29 +8,94 @@
88
*/
99
'use strict';
1010

11+
const fs = require('fs');
1112
const path = require('fast-path');
1213

14+
const NODE_MODULES_DIR = path.sep + 'node_modules' + path.sep;
15+
1316
class DependencyGraphHelpers {
14-
constructor({ providesModuleNodeModules, assetExts }) {
15-
this._providesModuleNodeModules = providesModuleNodeModules;
17+
constructor({
18+
roots = [],
19+
providesModuleNodeModules = [],
20+
assetExts = [],
21+
}) {
22+
this._hasteRegex = _buildHasteRegex(
23+
this._resolveHastePackages(providesModuleNodeModules, roots)
24+
);
1625
this._assetExts = assetExts;
1726
}
1827

28+
/**
29+
* An item in the `providesModuleNodeModule` array is an object containing two keys:
30+
* * 'name' - the package name
31+
* * 'parent' - if a string, it's the name of the parent package of the module in
32+
* the dep tree. If null, it signifies that we should look for this package at
33+
* the top level of the module tree.
34+
*
35+
* We try to resolve the specified module...but if we can't, we fallback to looking
36+
* in 'node_modules/[package name]' (this satisfies use cases where the actual
37+
* 'react-native' package is one of the root dirs, such as when we run tests
38+
* or examples).
39+
*/
40+
_resolveHastePackages(packages, roots) {
41+
const packagePathForPackage = ({ name, parent }, rootDir) => {
42+
let packagePath;
43+
if (parent) {
44+
if (!Array.isArray(parent)) {
45+
parent = [parent];
46+
}
47+
parent.push(name);
48+
packagePath = rootDir +
49+
NODE_MODULES_DIR +
50+
parent.join(NODE_MODULES_DIR);
51+
} else {
52+
packagePath = rootDir + NODE_MODULES_DIR + name;
53+
}
54+
55+
if (packagePath.endsWith(path.sep)) {
56+
return packagePath.slice(0, -1);
57+
} else {
58+
return packagePath;
59+
}
60+
};
61+
62+
const hastePackages = [];
63+
packages.forEach(p => {
64+
roots.forEach(rootDir => {
65+
const packagePath = packagePathForPackage(p, rootDir);
66+
try {
67+
const stats = fs.statSync(packagePath);
68+
if (stats && stats.isDirectory()) {
69+
hastePackages.push(packagePath);
70+
}
71+
} catch (e) {
72+
// if we don't find the package, let's just default to node_modules/[package name]
73+
hastePackages.push(packagePathForPackage({ name: p.name }, rootDir));
74+
}
75+
});
76+
});
77+
return hastePackages;
78+
}
79+
80+
/**
81+
* This method has three possible outcomes:
82+
*
83+
* 1) A file is not in 'node_modules' at all (some type of file in the project).
84+
* 2) The file is in 'node_modules', and it's contained in one of the
85+
* 'providesModuleNodeModules' packages.
86+
* 3) It's in 'node_modules' but not in a 'providesModuleNodeModule' package,
87+
* so it's just a normal node_module.
88+
*
89+
* This method uses a regex to do the directory testing, rather than for loop
90+
* and `indexOf` in order to get perf wins.
91+
*/
1992
isNodeModulesDir(file) {
20-
const index = file.lastIndexOf('/node_modules/');
93+
const index = file.indexOf(NODE_MODULES_DIR);
2194
if (index === -1) {
2295
return false;
2396
}
2497

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) {
29-
return false;
30-
}
31-
}
32-
33-
return true;
98+
return !this._hasteRegex.test(file);
3499
}
35100

36101
isAssetFile(file) {
@@ -42,4 +107,29 @@ class DependencyGraphHelpers {
42107
}
43108
}
44109

110+
/**
111+
* Given a list of directories, build a regex that takes the form:
112+
* ^((?![module's node_modules dir])[module dir])|
113+
* ((?![next module's node_modules dir])[next module dir])|...
114+
*
115+
* This is an alternative to looping through any of the providesModuleNodeModules
116+
* during `isNodeModulesDir`, which is run tens of thousands of times in a typical
117+
* project. A regex is much faster in this use-case.
118+
*/
119+
function _buildHasteRegex(dirs) {
120+
const dirRegexes = [];
121+
dirs.forEach(dir => {
122+
const dirRegex = '((?!' +
123+
escapeRegExp(dir + NODE_MODULES_DIR) + ')' +
124+
escapeRegExp(dir + path.sep) + ')';
125+
dirRegexes.push(dirRegex);
126+
});
127+
128+
return new RegExp('^' + dirRegexes.join('|'));
129+
}
130+
131+
function escapeRegExp(str) {
132+
return str.replace(/[\-\[\]\/\{\}\(\)\*\+\?\.\\\^\$\|]/g, '\\$&');
133+
}
134+
45135
module.exports = DependencyGraphHelpers;

‎src/DependencyGraph/__tests__/DependencyGraphHelpers-test.js

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
/**
2+
* Copyright (c) 2015-present, Facebook, Inc.
3+
* All rights reserved.
4+
*
5+
* This source code is licensed under the BSD-style license found in the
6+
* LICENSE file in the root directory of this source tree. An additional grant
7+
* of patent rights can be found in the PATENTS file in the same directory.
8+
*/
9+
'use strict';
10+
11+
jest.autoMockOff();
12+
13+
jest.mock('fs');
14+
15+
const DependencyGraphHelpers = require('../DependencyGraphHelpers');
16+
const fs = require('graceful-fs');
17+
18+
describe('DependencyGraphHelpers', function() {
19+
it('should properly resolve haste packages', function() {
20+
var root = '/root';
21+
_setMockFilesystem();
22+
23+
const helpers = new DependencyGraphHelpers({});
24+
const hastePackageDirs = helpers._resolveHastePackages([
25+
{ name: 'haste-fbjs', parent: 'react-native' },
26+
{ name: 'react-haste' },
27+
{ name: 'react-native' },
28+
], [root]);
29+
30+
expect(hastePackageDirs).toEqual([
31+
'/root/node_modules/react-native/node_modules/haste-fbjs',
32+
'/root/node_modules/react-haste',
33+
'/root/node_modules/react-native',
34+
]);
35+
});
36+
37+
it('should correctly determined whether a file is a node_module or haste module', function() {
38+
var root = '/root';
39+
_setMockFilesystem();
40+
41+
const helpers = new DependencyGraphHelpers({
42+
providesModuleNodeModules: [
43+
{ name: 'haste-fbjs', parent: 'react-native' },
44+
{ name: 'react-haste' },
45+
],
46+
roots: [root],
47+
});
48+
49+
expect(
50+
helpers.isNodeModulesDir('/root/index.js')
51+
).toBe(false);
52+
53+
expect(
54+
helpers.isNodeModulesDir('/root/node_modules/haste-fbjs/main.js')
55+
).toBe(true);
56+
57+
expect(
58+
helpers.isNodeModulesDir('/root/node_modules/react-native/node_modules/haste-fbjs/main.js')
59+
).toBe(false);
60+
});
61+
62+
function _setMockFilesystem() {
63+
fs.__setMockFilesystem({
64+
'root': {
65+
'index.js': [
66+
'/**',
67+
' * @providesModule index',
68+
' */',
69+
'require("shouldWork");',
70+
].join('\n'),
71+
'node_modules': {
72+
// A peer version of haste-fbjs
73+
'haste-fbjs': {
74+
'package.json': JSON.stringify({
75+
name: 'haste-fbjs',
76+
main: 'main.js',
77+
}),
78+
'main.js': [
79+
'/**',
80+
' * @providesModule shouldWork',
81+
' */',
82+
].join('\n'),
83+
},
84+
'react-native': {
85+
'package.json': JSON.stringify({
86+
name: 'react-native',
87+
main: 'main.js',
88+
}),
89+
'node_modules': {
90+
// the version of haste-fbjs that
91+
// we specified to be a `providesModuleNodeModule`
92+
'haste-fbjs': {
93+
'package.json': JSON.stringify({
94+
name: 'haste-fbjs',
95+
main: 'main.js',
96+
}),
97+
'main.js': [
98+
'/**',
99+
' * @providesModule shouldWork',
100+
' */',
101+
].join('\n'),
102+
},
103+
},
104+
},
105+
},
106+
},
107+
});
108+
}
109+
});

‎src/__mocks__/graceful-fs.js

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,43 @@ fs.stat.mockImpl(function(filepath, callback) {
112112
}
113113
});
114114

115+
fs.statSync.mockImpl(function(filepath) {
116+
var node;
117+
node = getToNode(filepath);
118+
119+
var mtime = {
120+
getTime: function() {
121+
return Math.ceil(Math.random() * 10000000);
122+
},
123+
};
124+
125+
if (node.SYMLINK) {
126+
return fs.statSync(node.SYMLINK);
127+
}
128+
129+
if (node && typeof node === 'object') {
130+
return {
131+
isDirectory: function() {
132+
return true;
133+
},
134+
isSymbolicLink: function() {
135+
return false;
136+
},
137+
mtime: mtime,
138+
};
139+
} else {
140+
return {
141+
isDirectory: function() {
142+
return false;
143+
},
144+
isSymbolicLink: function() {
145+
return false;
146+
},
147+
mtime: mtime,
148+
};
149+
}
150+
});
151+
115152
const noop = () => {};
116153
fs.open.mockImpl(function(path) {
117154
const callback = arguments[arguments.length - 1] || noop;

0 commit comments

Comments
 (0)