Respect transitive deps when using nodeModulesPaths (facebook#738)

Summary:
**Summary**
This fixes facebook#737 take a look at the issue for more info. I've also added a test case to hopefully catch this in the future. The order of the paths is important in this change.

This change changes the resolution order slightly, we can see this by observing `allDirPaths` https://github.com/facebook/metro/blob/02fade69cad95488e1afba0c3dba8e25d8453bc5/packages/metro-resolver/src/resolve.js#L126-L128

Without this change, the `allDirPaths` looks like this:
```
    [
      '/other-root/node_modules/banana-module',
      '/other-root/node_modules/banana/node_modules/banana-module',
      '/other-root/node_modules/node_modules/banana-module',
      '/other-root/node_modules/banana-module',
      '/node_modules/banana-module'
    ]
```

See how the `banana-module` nested inside `banana` is second & not first.

With this change `allDirPaths` is update to this:
```
    [
      '/other-root/node_modules/banana/node_modules/banana-module',
      '/other-root/node_modules/node_modules/banana-module',
      '/other-root/node_modules/banana-module',
      '/node_modules/banana-module',
      '/other-root/node_modules/banana-module'
    ]
```

We correctly attempt to resolve the transitive `banana-module` dependency first.

**Test plan**
Added a unit test

Pull Request resolved: facebook#738

Reviewed By: motiz88

Differential Revision: D32310356

Pulled By: GijsWeterings

fbshipit-source-id: 158e401cb47cc4a4bcc11687ef7e34caa8ce3dde

packages/metro-resolver/src/__tests__/index-test.js

@@ -57,9 +57,19 @@ const CONTEXT: ResolutionContext = (() => {
       },
       'other-root': {
         node_modules: {
+          'banana-module': {
+            'package.json': true,
+            'main.js': true,
+          },
           banana: {
             'package.json': true,
             'main.js': true,
+            node_modules: {
+              'banana-module': {
+                'package.json': true,
+                'main.js': true,
+              },
+            },
           },
         },
       },
@@ -218,14 +228,92 @@ it('uses `nodeModulesPaths` to find additional node_modules not in the direct pa
   expect(() => Resolver.resolve(context, 'kiwi', null))
     .toThrowErrorMatchingInlineSnapshot(`
     "Module does not exist in the Haste module map or in these directories:
-      /other-root/node_modules
       /root/project/node_modules
       /root/node_modules
       /node_modules
+      /other-root/node_modules
     "
   `);
 });
 
+it('resolves transitive dependencies when using `nodeModulesPaths`', () => {
+  const context = Object.assign(
+    {},
+    {...CONTEXT, originModulePath: '/other-root/node_modules/banana/main.js'},
+    {
+      nodeModulesPaths: ['/other-root/node_modules'],
+    },
+  );
+
+  expect(Resolver.resolve(context, 'banana-module', null)).toEqual({
+    type: 'sourceFile',
+    filePath:
+      '/other-root/node_modules/banana/node_modules/banana-module/main.js',
+  });
+
+  expect(Resolver.resolve(context, 'banana-module', null)).not.toEqual({
+    type: 'sourceFile',
+    filePath: '/other-root/node_modules/banana-module/main.js',
+  });
+});
+
+describe('disableHierarchicalLookup', () => {
+  const context = Object.assign({}, CONTEXT, {
+    disableHierarchicalLookup: true,
+  });
+
+  it('disables node_modules lookup', () => {
+    expect(() => Resolver.resolve(context, 'apple', null))
+      .toThrowErrorMatchingInlineSnapshot(`
+      "Module does not exist in the Haste module map
+
+      "
+    `);
+  });
+
+  it('respects nodeModulesPaths', () => {
+    const contextWithOtherRoot = {
+      ...context,
+      nodeModulesPaths: ['/other-root/node_modules'],
+    };
+
+    // apple exists in /root/node_modules
+    expect(() => Resolver.resolve(contextWithOtherRoot, 'apple', null))
+      .toThrowErrorMatchingInlineSnapshot(`
+      "Module does not exist in the Haste module map or in these directories:
+        /other-root/node_modules
+      "
+    `);
+
+    expect(Resolver.resolve(contextWithOtherRoot, 'banana', null)).toEqual({
+      type: 'sourceFile',
+      filePath: '/other-root/node_modules/banana/main.js',
+    });
+
+    // kiwi doesn't exist anywhere
+    expect(() => Resolver.resolve(contextWithOtherRoot, 'kiwi', null))
+      .toThrowErrorMatchingInlineSnapshot(`
+      "Module does not exist in the Haste module map or in these directories:
+        /other-root/node_modules
+      "
+    `);
+  });
+
+  it('respects extraNodeModules', () => {
+    const contextWithExtra = {
+      ...context,
+      extraNodeModules: {
+        'renamed-apple': '/root/node_modules/apple',
+      },
+    };
+
+    expect(Resolver.resolve(contextWithExtra, 'renamed-apple', null)).toEqual({
+      type: 'sourceFile',
+      filePath: '/root/node_modules/apple/main.js',
+    });
+  });
+});
+
 it('resolves Haste modules', () => {
   expect(Resolver.resolve(CONTEXT, 'Foo', null)).toEqual({
     type: 'sourceFile',

packages/metro-resolver/src/resolve.js

@@ -91,14 +91,21 @@ function resolve(
     } catch (error) {}
   }
 
-  const nodeModulesPaths = Array.from(context.nodeModulesPaths);
+  const nodeModulesPaths = [];
   let next = path.dirname(originModulePath);
-  let candidate;
-  do {
-    candidate = next;
-    nodeModulesPaths.push(path.join(candidate, 'node_modules'));
-    next = path.dirname(candidate);
-  } while (candidate !== next);
+  const {disableHierarchicalLookup} = context;
+
+  if (!disableHierarchicalLookup) {
+    let candidate;
+    do {
+      candidate = next;
+      nodeModulesPaths.push(path.join(candidate, 'node_modules'));
+      next = path.dirname(candidate);
+    } while (candidate !== next);
+  }
+
+  // Fall back to `nodeModulesPaths` after hierarchical lookup, similar to $NODE_PATH
+  nodeModulesPaths.push(...context.nodeModulesPaths);
 
   const extraPaths = [];
   const {extraNodeModules} = context;