Skip to content

Commit 6cec263

Browse files
skevyfacebook-github-bot-5
skevy
authored and
facebook-github-bot-5
committed
Fix @providesModule not being ignored properly
Summary: There's quite a bit of code scattered around the packager regarding ignoring the `providesModule` Haste pragma in any file that isn't in `react-native`, `react-tools` or `parse`. There is even a (passing) test case. However, there's an edge case. Take, for example, `fbjs`. It has a module inside of it called `ErrorUtils`. `react-relay` requires this file normally, in Common.JS style, by doing `require('fbjs/libs/ErrorUtils')`. But when `react-native` attempts to require `ErrorUtils` using the HasteModule format (in it's JavaScript initialization), it resolves the `fbjs` `ErrorUtils` module, instead of RN's `ErrorUtils`. This happens, it turns out, because when a module is read (in `Module._read`), it's not caring about whether or not it should pay attention to `providesModule`, and is just assigning the `providesModule` value as the id of the module no matter what. Then when `Module.getName` is called, it will always use that `data.id` that was set, thus creating the wrong dependency tree. This Closes facebook/react-native#3625 Reviewed By: svcscm Differential Revision: D2632317 Pulled By: vjeux fb-gh-sync-id: efd8066eaf6f18fcf79698beab36cab90bf5cd6d
1 parent 5988591 commit 6cec263

11 files changed

Lines changed: 140 additions & 54 deletions

File tree

‎packager/react-packager/src/DependencyResolver/AssetModule.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ class AssetModule extends Module {
2525
return Promise.resolve([]);
2626
}
2727

28-
_read() {
28+
read() {
2929
return Promise.resolve({});
3030
}
3131

packager/react-packager/src/DependencyResolver/DependencyGraph/Helpers.js renamed to packager/react-packager/src/DependencyResolver/DependencyGraph/DependencyGraphHelpers.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010

1111
const path = require('path');
1212

13-
class Helpers {
13+
class DependencyGraphHelpers {
1414
constructor({ providesModuleNodeModules, assetExts }) {
1515
this._providesModuleNodeModules = providesModuleNodeModules;
1616
this._assetExts = assetExts;
@@ -46,4 +46,4 @@ class Helpers {
4646
}
4747
}
4848

49-
module.exports = Helpers;
49+
module.exports = DependencyGraphHelpers;

‎packager/react-packager/src/DependencyResolver/DependencyGraph/DeprecatedAssetMap.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ class DeprecatedAssetMap {
9292
debug('Conflcting assets', name);
9393
}
9494

95-
this._map[name] = new AssetModule_DEPRECATED(file);
95+
this._map[name] = new AssetModule_DEPRECATED({ file });
9696
}
9797
}
9898

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

Lines changed: 74 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2362,6 +2362,7 @@ describe('DependencyGraph', function() {
23622362

23632363
pit('should selectively ignore providesModule in node_modules', function() {
23642364
var root = '/root';
2365+
var otherRoot = '/anotherRoot';
23652366
fs.__setMockFilesystem({
23662367
'root': {
23672368
'index.js': [
@@ -2371,13 +2372,17 @@ describe('DependencyGraph', function() {
23712372
'require("shouldWork");',
23722373
'require("dontWork");',
23732374
'require("wontWork");',
2375+
'require("ember");',
2376+
'require("internalVendoredPackage");',
2377+
'require("anotherIndex");',
23742378
].join('\n'),
23752379
'node_modules': {
23762380
'react-haste': {
23772381
'package.json': JSON.stringify({
23782382
name: 'react-haste',
23792383
main: 'main.js',
23802384
}),
2385+
// @providesModule should not be ignored here, because react-haste is whitelisted
23812386
'main.js': [
23822387
'/**',
23832388
' * @providesModule shouldWork',
@@ -2390,6 +2395,7 @@ describe('DependencyGraph', function() {
23902395
name: 'bar',
23912396
main: 'main.js',
23922397
}),
2398+
// @providesModule should be ignored here, because it's not whitelisted
23932399
'main.js':[
23942400
'/**',
23952401
' * @providesModule dontWork',
@@ -2411,28 +2417,63 @@ describe('DependencyGraph', function() {
24112417
name: 'ember',
24122418
main: 'main.js',
24132419
}),
2420+
// @providesModule should be ignored here, because it's not whitelisted,
2421+
// and also, the modules "id" should be ember/main.js, not it's haste name
24142422
'main.js':[
24152423
'/**',
24162424
' * @providesModule wontWork',
24172425
' */',
24182426
'hi();',
2419-
].join('\n'),
2427+
].join('\n')
24202428
},
24212429
},
2430+
// This part of the dep graph is meant to emulate internal facebook infra.
2431+
// By whitelisting `vendored_modules`, haste should still work.
2432+
'vendored_modules': {
2433+
'a-vendored-package': {
2434+
'package.json': JSON.stringify({
2435+
name: 'a-vendored-package',
2436+
main: 'main.js',
2437+
}),
2438+
// @providesModule should _not_ be ignored here, because it's whitelisted.
2439+
'main.js':[
2440+
'/**',
2441+
' * @providesModule internalVendoredPackage',
2442+
' */',
2443+
'hiFromInternalPackage();',
2444+
].join('\n'),
2445+
}
2446+
},
24222447
},
2448+
// we need to support multiple roots and using haste between them
2449+
'anotherRoot': {
2450+
'index.js': [
2451+
'/**',
2452+
' * @providesModule anotherIndex',
2453+
' */',
2454+
'wazup()',
2455+
].join('\n'),
2456+
}
24232457
});
24242458

24252459
var dgraph = new DependencyGraph({
24262460
...defaults,
2427-
roots: [root],
2461+
roots: [root, otherRoot],
24282462
});
24292463
return getOrderedDependenciesAsJSON(dgraph, '/root/index.js').then(function(deps) {
24302464
expect(deps)
24312465
.toEqual([
24322466
{
24332467
id: 'index',
24342468
path: '/root/index.js',
2435-
dependencies: ['shouldWork', 'dontWork', 'wontWork'],
2469+
dependencies: [
2470+
'shouldWork',
2471+
'dontWork',
2472+
'wontWork',
2473+
'ember',
2474+
'internalVendoredPackage',
2475+
'anotherIndex'
2476+
],
24362477
isAsset: false,
24372478
isAsset_DEPRECATED: false,
24382479
isJSON: false,
@@ -2459,6 +2500,36 @@ describe('DependencyGraph', function() {
24592500
isPolyfill: false,
24602501
resolution: undefined,
24612502
},
2503+
{
2504+
id: 'ember/main.js',
2505+
path: '/root/node_modules/ember/main.js',
2506+
dependencies: [],
2507+
isAsset: false,
2508+
isAsset_DEPRECATED: false,
2509+
isJSON: false,
2510+
isPolyfill: false,
2511+
resolution: undefined,
2512+
},
2513+
{
2514+
id: 'internalVendoredPackage',
2515+
path: '/root/vendored_modules/a-vendored-package/main.js',
2516+
dependencies: [],
2517+
isAsset: false,
2518+
isAsset_DEPRECATED: false,
2519+
isJSON: false,
2520+
isPolyfill: false,
2521+
resolution: undefined,
2522+
},
2523+
{
2524+
id: 'anotherIndex',
2525+
path: '/anotherRoot/index.js',
2526+
dependencies: [],
2527+
isAsset: false,
2528+
isAsset_DEPRECATED: false,
2529+
isJSON: false,
2530+
isPolyfill: false,
2531+
resolution: undefined,
2532+
},
24622533
]);
24632534
});
24642535
});

‎packager/react-packager/src/DependencyResolver/DependencyGraph/index.js‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ const getPlatformExtension = require('../lib/getPlatformExtension');
1616
const isAbsolutePath = require('absolute-path');
1717
const path = require('path');
1818
const util = require('util');
19-
const Helpers = require('./Helpers');
19+
const DependencyGraphHelpers = require('./DependencyGraphHelpers');
2020
const ResolutionRequest = require('./ResolutionRequest');
2121
const ResolutionResponse = require('./ResolutionResponse');
2222
const HasteMap = require('./HasteMap');
@@ -57,7 +57,7 @@ class DependencyGraph {
5757
extractRequires,
5858
};
5959
this._cache = this._opts.cache;
60-
this._helpers = new Helpers(this._opts);
60+
this._helpers = new DependencyGraphHelpers(this._opts);
6161
this.load().catch((err) => {
6262
// This only happens at initialization. Live errors are easier to recover from.
6363
console.error('Error building DependencyGraph:\n', err.stack);
@@ -97,7 +97,8 @@ class DependencyGraph {
9797
this._moduleCache = new ModuleCache(
9898
this._fastfs,
9999
this._cache,
100-
this._opts.extractRequires
100+
this._opts.extractRequires,
101+
this._helpers
101102
);
102103

103104
this._hasteMap = new HasteMap({

‎packager/react-packager/src/DependencyResolver/Module.js‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ const extractRequires = require('./lib/extractRequires');
1515

1616
class Module {
1717

18-
constructor(file, fastfs, moduleCache, cache, extractor) {
18+
constructor({ file, fastfs, moduleCache, cache, extractor, depGraphHelpers }) {
1919
if (!isAbsolutePath(file)) {
2020
throw new Error('Expected file to be absolute path but got ' + file);
2121
}
@@ -27,17 +27,18 @@ class Module {
2727
this._moduleCache = moduleCache;
2828
this._cache = cache;
2929
this._extractor = extractor;
30+
this._depGraphHelpers = depGraphHelpers;
3031
}
3132

3233
isHaste() {
33-
return this._read().then(data => !!data.id);
34+
return this.read().then(data => !!data.id);
3435
}
3536

3637
getName() {
3738
return this._cache.get(
3839
this.path,
3940
'name',
40-
() => this._read().then(data => {
41+
() => this.read().then(data => {
4142
if (data.id) {
4243
return data.id;
4344
}
@@ -66,23 +67,31 @@ class Module {
6667
}
6768

6869
getDependencies() {
69-
return this._read().then(data => data.dependencies);
70+
return this.read().then(data => data.dependencies);
7071
}
7172

7273
getAsyncDependencies() {
73-
return this._read().then(data => data.asyncDependencies);
74+
return this.read().then(data => data.asyncDependencies);
7475
}
7576

7677
invalidate() {
7778
this._cache.invalidate(this.path);
7879
}
7980

80-
_read() {
81+
read() {
8182
if (!this._reading) {
8283
this._reading = this._fastfs.readFile(this.path).then(content => {
8384
const data = {};
85+
86+
// Set an id on the module if it's using @providesModule syntax
87+
// and if it's NOT in node_modules (and not a whitelisted node_module).
88+
// This handles the case where a project may have a dep that has @providesModule
89+
// docblock comments, but doesn't want it to conflict with whitelisted @providesModule
90+
// modules, such as react-haste, fbjs-haste, or react-native or with non-dependency,
91+
// project-specific code that is using @providesModule.
8492
const moduleDocBlock = docblock.parseAsObject(content);
85-
if (moduleDocBlock.providesModule || moduleDocBlock.provides) {
93+
if (!this._depGraphHelpers.isNodeModulesDir(this.path) &&
94+
(moduleDocBlock.providesModule || moduleDocBlock.provides)) {
8695
data.id = /^(\S*)/.exec(
8796
moduleDocBlock.providesModule || moduleDocBlock.provides
8897
)[1];

‎packager/react-packager/src/DependencyResolver/ModuleCache.js‎

Lines changed: 21 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -7,50 +7,52 @@ const path = require('path');
77

88
class ModuleCache {
99

10-
constructor(fastfs, cache, extractRequires) {
10+
constructor(fastfs, cache, extractRequires, depGraphHelpers) {
1111
this._moduleCache = Object.create(null);
1212
this._packageCache = Object.create(null);
1313
this._fastfs = fastfs;
1414
this._cache = cache;
1515
this._extractRequires = extractRequires;
16+
this._depGraphHelpers = depGraphHelpers;
1617
fastfs.on('change', this._processFileChange.bind(this));
1718
}
1819

1920
getModule(filePath) {
2021
filePath = path.resolve(filePath);
2122
if (!this._moduleCache[filePath]) {
22-
this._moduleCache[filePath] = new Module(
23-
filePath,
24-
this._fastfs,
25-
this,
26-
this._cache,
27-
this._extractRequires
28-
);
23+
this._moduleCache[filePath] = new Module({
24+
file: filePath,
25+
fastfs: this._fastfs,
26+
moduleCache: this,
27+
cache: this._cache,
28+
extractor: this._extractRequires,
29+
depGraphHelpers: this._depGraphHelpers
30+
});
2931
}
3032
return this._moduleCache[filePath];
3133
}
3234

3335
getAssetModule(filePath) {
3436
filePath = path.resolve(filePath);
3537
if (!this._moduleCache[filePath]) {
36-
this._moduleCache[filePath] = new AssetModule(
37-
filePath,
38-
this._fastfs,
39-
this,
40-
this._cache,
41-
);
38+
this._moduleCache[filePath] = new AssetModule({
39+
file: filePath,
40+
fastfs: this._fastfs,
41+
moduleCache: this,
42+
cache: this._cache,
43+
});
4244
}
4345
return this._moduleCache[filePath];
4446
}
4547

4648
getPackage(filePath) {
4749
filePath = path.resolve(filePath);
4850
if (!this._packageCache[filePath]) {
49-
this._packageCache[filePath] = new Package(
50-
filePath,
51-
this._fastfs,
52-
this._cache,
53-
);
51+
this._packageCache[filePath] = new Package({
52+
file: filePath,
53+
fastfs: this._fastfs,
54+
cache: this._cache,
55+
});
5456
}
5557
return this._packageCache[filePath];
5658
}

‎packager/react-packager/src/DependencyResolver/Package.js‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ const path = require('path');
55

66
class Package {
77

8-
constructor(file, fastfs, cache) {
8+
constructor({ file, fastfs, cache }) {
99
this.path = path.resolve(file);
1010
this.root = path.dirname(this.path);
1111
this._fastfs = fastfs;
@@ -14,7 +14,7 @@ class Package {
1414
}
1515

1616
getMain() {
17-
return this._read().then(json => {
17+
return this.read().then(json => {
1818
var replacements = getReplacements(json);
1919
if (typeof replacements === 'string') {
2020
return path.join(this.root, replacements);
@@ -36,13 +36,13 @@ class Package {
3636

3737
isHaste() {
3838
return this._cache.get(this.path, 'package-haste', () =>
39-
this._read().then(json => !!json.name)
39+
this.read().then(json => !!json.name)
4040
);
4141
}
4242

4343
getName() {
4444
return this._cache.get(this.path, 'package-name', () =>
45-
this._read().then(json => json.name)
45+
this.read().then(json => json.name)
4646
);
4747
}
4848

@@ -51,7 +51,7 @@ class Package {
5151
}
5252

5353
redirectRequire(name) {
54-
return this._read().then(json => {
54+
return this.read().then(json => {
5555
var replacements = getReplacements(json);
5656

5757
if (!replacements || typeof replacements !== 'object') {
@@ -81,7 +81,7 @@ class Package {
8181
});
8282
}
8383

84-
_read() {
84+
read() {
8585
if (!this._reading) {
8686
this._reading = this._fastfs.readFile(this.path)
8787
.then(jsonStr => JSON.parse(jsonStr));

‎packager/react-packager/src/DependencyResolver/Polyfill.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ const Module = require('./Module');
55

66
class Polyfill extends Module {
77
constructor({ path, id, dependencies }) {
8-
super(path);
8+
super({ file: path });
99
this._id = id;
1010
this._dependencies = dependencies;
1111
}

0 commit comments

Comments
 (0)