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 https://github.com/facebook/react-native/pull/3625

Reviewed By: svcscm

Differential Revision: D2632317

Pulled By: vjeux

fb-gh-sync-id: efd8066eaf6f18fcf79698beab36cab90bf5cd6d

react-packager/src/DependencyResolver/AssetModule.js

@@ -25,7 +25,7 @@ class AssetModule extends Module {
     return Promise.resolve([]);
   }
 
-  _read() {
+  read() {
     return Promise.resolve({});
   }
 

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

@@ -10,7 +10,7 @@
 
 const path = require('path');
 
-class Helpers {
+class DependencyGraphHelpers {
   constructor({ providesModuleNodeModules, assetExts }) {
     this._providesModuleNodeModules = providesModuleNodeModules;
     this._assetExts = assetExts;
@@ -46,4 +46,4 @@ class Helpers {
   }
 }
 
-module.exports = Helpers;
+module.exports = DependencyGraphHelpers;

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

@@ -92,7 +92,7 @@ class DeprecatedAssetMap {
         debug('Conflcting assets', name);
       }
 
-      this._map[name] = new AssetModule_DEPRECATED(file);
+      this._map[name] = new AssetModule_DEPRECATED({ file });
     }
   }
 

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

@@ -2362,6 +2362,7 @@ describe('DependencyGraph', function() {
 
     pit('should selectively ignore providesModule in node_modules', function() {
       var root = '/root';
+      var otherRoot = '/anotherRoot';
       fs.__setMockFilesystem({
         'root': {
           'index.js': [
@@ -2371,6 +2372,9 @@ describe('DependencyGraph', function() {
             'require("shouldWork");',
             'require("dontWork");',
             'require("wontWork");',
+            'require("ember");',
+            'require("internalVendoredPackage");',
+            'require("anotherIndex");',
           ].join('\n'),
           'node_modules': {
             'react-haste': {
@@ -2378,6 +2382,7 @@ describe('DependencyGraph', function() {
                 name: 'react-haste',
                 main: 'main.js',
               }),
+              // @providesModule should not be ignored here, because react-haste is whitelisted
               'main.js': [
                 '/**',
                 ' * @providesModule shouldWork',
@@ -2390,6 +2395,7 @@ describe('DependencyGraph', function() {
                     name: 'bar',
                     main: 'main.js',
                   }),
+                  // @providesModule should be ignored here, because it's not whitelisted
                   'main.js':[
                     '/**',
                     ' * @providesModule dontWork',
@@ -2411,20 +2417,48 @@ describe('DependencyGraph', function() {
                 name: 'ember',
                 main: 'main.js',
               }),
+              // @providesModule should be ignored here, because it's not whitelisted,
+              // and also, the modules "id" should be ember/main.js, not it's haste name
               'main.js':[
                 '/**',
                 ' * @providesModule wontWork',
                 ' */',
                 'hi();',
+              ].join('\n')
+            },
+          },
+          // This part of the dep graph is meant to emulate internal facebook infra.
+          // By whitelisting `vendored_modules`, haste should still work.
+          'vendored_modules': {
+            'a-vendored-package': {
+              'package.json': JSON.stringify({
+                name: 'a-vendored-package',
+                main: 'main.js',
+              }),
+              // @providesModule should _not_ be ignored here, because it's whitelisted.
+              'main.js':[
+                '/**',
+                ' * @providesModule internalVendoredPackage',
+                ' */',
+                'hiFromInternalPackage();',
               ].join('\n'),
+            }
           },
         },
-        },
+        // we need to support multiple roots and using haste between them
+        'anotherRoot': {
+          'index.js': [
+            '/**',
+            ' * @providesModule anotherIndex',
+            ' */',
+            'wazup()',
+          ].join('\n'),
+        }
       });
 
       var dgraph = new DependencyGraph({
         ...defaults,
-        roots: [root],
+        roots: [root, otherRoot],
       });
       return getOrderedDependenciesAsJSON(dgraph, '/root/index.js').then(function(deps) {
         expect(deps)
@@ -2432,7 +2466,14 @@ describe('DependencyGraph', function() {
             {
               id: 'index',
               path: '/root/index.js',
-              dependencies: ['shouldWork', 'dontWork', 'wontWork'],
+              dependencies: [
+                'shouldWork',
+                'dontWork',
+                'wontWork',
+                'ember',
+                'internalVendoredPackage',
+                'anotherIndex'
+              ],
               isAsset: false,
               isAsset_DEPRECATED: false,
               isJSON: false,
@@ -2459,6 +2500,36 @@ describe('DependencyGraph', function() {
               isPolyfill: false,
               resolution: undefined,
             },
+            {
+              id: 'ember/main.js',
+              path: '/root/node_modules/ember/main.js',
+              dependencies: [],
+              isAsset: false,
+              isAsset_DEPRECATED: false,
+              isJSON: false,
+              isPolyfill: false,
+              resolution: undefined,
+            },
+            {
+              id: 'internalVendoredPackage',
+              path: '/root/vendored_modules/a-vendored-package/main.js',
+              dependencies: [],
+              isAsset: false,
+              isAsset_DEPRECATED: false,
+              isJSON: false,
+              isPolyfill: false,
+              resolution: undefined,
+            },
+            {
+              id: 'anotherIndex',
+              path: '/anotherRoot/index.js',
+              dependencies: [],
+              isAsset: false,
+              isAsset_DEPRECATED: false,
+              isJSON: false,
+              isPolyfill: false,
+              resolution: undefined,
+            },
           ]);
       });
     });

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

@@ -16,7 +16,7 @@ const getPlatformExtension = require('../lib/getPlatformExtension');
 const isAbsolutePath = require('absolute-path');
 const path = require('path');
 const util = require('util');
-const Helpers = require('./Helpers');
+const DependencyGraphHelpers = require('./DependencyGraphHelpers');
 const ResolutionRequest = require('./ResolutionRequest');
 const ResolutionResponse = require('./ResolutionResponse');
 const HasteMap = require('./HasteMap');
@@ -57,7 +57,7 @@ class DependencyGraph {
       extractRequires,
     };
     this._cache = this._opts.cache;
-    this._helpers = new Helpers(this._opts);
+    this._helpers = new DependencyGraphHelpers(this._opts);
     this.load().catch((err) => {
       // This only happens at initialization. Live errors are easier to recover from.
       console.error('Error building DependencyGraph:\n', err.stack);
@@ -97,7 +97,8 @@ class DependencyGraph {
     this._moduleCache = new ModuleCache(
       this._fastfs,
       this._cache,
-      this._opts.extractRequires
+      this._opts.extractRequires,
+      this._helpers
     );
 
     this._hasteMap = new HasteMap({

react-packager/src/DependencyResolver/Module.js

@@ -15,7 +15,7 @@ const extractRequires = require('./lib/extractRequires');
 
 class Module {
 
-  constructor(file, fastfs, moduleCache, cache, extractor) {
+  constructor({ file, fastfs, moduleCache, cache, extractor, depGraphHelpers }) {
     if (!isAbsolutePath(file)) {
       throw new Error('Expected file to be absolute path but got ' + file);
     }
@@ -27,17 +27,18 @@ class Module {
     this._moduleCache = moduleCache;
     this._cache = cache;
     this._extractor = extractor;
+    this._depGraphHelpers = depGraphHelpers;
   }
 
   isHaste() {
-    return this._read().then(data => !!data.id);
+    return this.read().then(data => !!data.id);
   }
 
   getName() {
     return this._cache.get(
       this.path,
       'name',
-      () => this._read().then(data => {
+      () => this.read().then(data => {
         if (data.id) {
           return data.id;
         }
@@ -66,23 +67,31 @@ class Module {
   }
 
   getDependencies() {
-    return this._read().then(data => data.dependencies);
+    return this.read().then(data => data.dependencies);
   }
 
   getAsyncDependencies() {
-    return this._read().then(data => data.asyncDependencies);
+    return this.read().then(data => data.asyncDependencies);
   }
 
   invalidate() {
     this._cache.invalidate(this.path);
   }
 
-  _read() {
+  read() {
     if (!this._reading) {
       this._reading = this._fastfs.readFile(this.path).then(content => {
         const data = {};
+
+        // Set an id on the module if it's using @providesModule syntax
+        // and if it's NOT in node_modules (and not a whitelisted node_module).
+        // This handles the case where a project may have a dep that has @providesModule
+        // docblock comments, but doesn't want it to conflict with whitelisted @providesModule
+        // modules, such as react-haste, fbjs-haste, or react-native or with non-dependency,
+        // project-specific code that is using @providesModule.
         const moduleDocBlock = docblock.parseAsObject(content);
-        if (moduleDocBlock.providesModule || moduleDocBlock.provides) {
+        if (!this._depGraphHelpers.isNodeModulesDir(this.path) &&
+            (moduleDocBlock.providesModule || moduleDocBlock.provides)) {
           data.id = /^(\S*)/.exec(
             moduleDocBlock.providesModule || moduleDocBlock.provides
           )[1];

react-packager/src/DependencyResolver/ModuleCache.js

@@ -7,25 +7,27 @@ const path = require('path');
 
 class ModuleCache {
 
-  constructor(fastfs, cache, extractRequires) {
+  constructor(fastfs, cache, extractRequires, depGraphHelpers) {
     this._moduleCache = Object.create(null);
     this._packageCache = Object.create(null);
     this._fastfs = fastfs;
     this._cache = cache;
     this._extractRequires = extractRequires;
+    this._depGraphHelpers = depGraphHelpers;
     fastfs.on('change', this._processFileChange.bind(this));
   }
 
   getModule(filePath) {
     filePath = path.resolve(filePath);
     if (!this._moduleCache[filePath]) {
-      this._moduleCache[filePath] = new Module(
+      this._moduleCache[filePath] = new Module({
-        filePath,
+        file: filePath,
-        this._fastfs,
+        fastfs: this._fastfs,
-        this,
+        moduleCache: this,
-        this._cache,
+        cache: this._cache,
-        this._extractRequires
+        extractor: this._extractRequires,
-      );
+        depGraphHelpers: this._depGraphHelpers
+      });
     }
     return this._moduleCache[filePath];
   }
@@ -33,12 +35,12 @@ class ModuleCache {
   getAssetModule(filePath) {
     filePath = path.resolve(filePath);
     if (!this._moduleCache[filePath]) {
-      this._moduleCache[filePath] = new AssetModule(
+      this._moduleCache[filePath] = new AssetModule({
-        filePath,
+        file: filePath,
-        this._fastfs,
+        fastfs: this._fastfs,
-        this,
+        moduleCache: this,
-        this._cache,
+        cache: this._cache,
-      );
+      });
     }
     return this._moduleCache[filePath];
   }
@@ -46,11 +48,11 @@ class ModuleCache {
   getPackage(filePath) {
     filePath = path.resolve(filePath);
     if (!this._packageCache[filePath]) {
-      this._packageCache[filePath] = new Package(
+      this._packageCache[filePath] = new Package({
-        filePath,
+        file: filePath,
-        this._fastfs,
+        fastfs: this._fastfs,
-        this._cache,
+        cache: this._cache,
-      );
+      });
     }
     return this._packageCache[filePath];
   }

react-packager/src/DependencyResolver/Package.js

@@ -5,7 +5,7 @@ const path = require('path');
 
 class Package {
 
-  constructor(file, fastfs, cache) {
+  constructor({ file, fastfs, cache }) {
     this.path = path.resolve(file);
     this.root = path.dirname(this.path);
     this._fastfs = fastfs;
@@ -14,7 +14,7 @@ class Package {
   }
 
   getMain() {
-    return this._read().then(json => {
+    return this.read().then(json => {
       var replacements = getReplacements(json);
       if (typeof replacements === 'string') {
         return path.join(this.root, replacements);
@@ -36,13 +36,13 @@ class Package {
 
   isHaste() {
     return this._cache.get(this.path, 'package-haste', () =>
-      this._read().then(json => !!json.name)
+      this.read().then(json => !!json.name)
     );
   }
 
   getName() {
     return this._cache.get(this.path, 'package-name', () =>
-      this._read().then(json => json.name)
+      this.read().then(json => json.name)
     );
   }
 
@@ -51,7 +51,7 @@ class Package {
   }
 
   redirectRequire(name) {
-    return this._read().then(json => {
+    return this.read().then(json => {
       var replacements = getReplacements(json);
 
       if (!replacements || typeof replacements !== 'object') {
@@ -81,7 +81,7 @@ class Package {
     });
   }
 
-  _read() {
+  read() {
     if (!this._reading) {
       this._reading = this._fastfs.readFile(this.path)
         .then(jsonStr => JSON.parse(jsonStr));

react-packager/src/DependencyResolver/Polyfill.js

@@ -5,7 +5,7 @@ const Module = require('./Module');
 
 class Polyfill extends Module {
   constructor({ path, id, dependencies }) {
-    super(path);
+    super({ file: path });
     this._id = id;
     this._dependencies = dependencies;
   }

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

@@ -22,6 +22,7 @@ jest
 const Fastfs = require('../fastfs');
 const Module = require('../Module');
 const ModuleCache = require('../ModuleCache');
+const DependencyGraphHelpers = require('../DependencyGraph/DependencyGraphHelpers');
 const Promise = require('promise');
 const fs = require('fs');
 
@@ -50,12 +51,13 @@ describe('Module', () => {
       const cache = new Cache();
 
       return fastfs.build().then(() => {
-        const module = new Module(
+        const module = new Module({
-          '/root/index.js',
+          file: '/root/index.js',
           fastfs,
-          new ModuleCache(fastfs, cache),
+          moduleCache: new ModuleCache(fastfs, cache),
-          cache
+          cache: cache,
-        );
+          depGraphHelpers: new DependencyGraphHelpers()
+        });
 
         return module.getAsyncDependencies().then(actual =>
           expect(actual).toEqual(expected)
@@ -122,13 +124,14 @@ describe('Module', () => {
       const cache = new Cache();
 
       return fastfs.build().then(() => {
-        return new Module(
+        return new Module({
-          '/root/index.js',
+          file: '/root/index.js',
           fastfs,
-          new ModuleCache(fastfs, cache),
+          moduleCache: new ModuleCache(fastfs, cache),
           cache,
-          extractor
+          extractor,
-        );
+          depGraphHelpers: new DependencyGraphHelpers()
+        });
       });
     }
 

react-packager/src/Resolver/__tests__/Resolver-test.js

@@ -51,7 +51,7 @@ describe('Resolver', function() {
   }
 
   function createModule(id, dependencies) {
-    var module = new Module();
+    var module = new Module({});
     module.getName.mockImpl(() => Promise.resolve(id));
     module.getDependencies.mockImpl(() => Promise.resolve(dependencies));
     return module;