[metro-resolver] Respect transitive deps when using `nodeModulesPaths` #738

sharmilajesupaul · 2021-11-10T01:08:23Z

Summary
This fixes #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

metro/packages/metro-resolver/src/resolve.js

Lines 126 to 128 in 02fade6

    
           const allDirPaths = nodeModulesPaths 
        
             .map(nodeModulePath => path.join(nodeModulePath, realModuleName)) 
        
             .concat(extraPaths);

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

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. 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 `banan-module` dependency first.

facebook-github-bot · 2021-11-10T01:08:27Z

Hi @sharmilajesupaul!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

facebook-github-bot · 2021-11-10T01:17:42Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

facebook-github-bot · 2021-11-10T10:22:41Z

@GijsWeterings has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

raejin

Looks great! Thanks for fixing this!

raejin · 2021-11-10T17:29:17Z

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

+              'banana-module': {
+                'package.json': true,
+                'main.js': true,
+              },


I like the example when you suggested a regular module under banana/node_modules/other-module/main.js that can also be resolved from banana/main.js.

raejin · 2021-11-10T17:29:50Z

cc @motiz88

motiz88 · 2021-11-10T18:27:44Z

I'm working on moving an internal call site away from nodeModulesPaths so we can land this. This has been on my radar in recent months - thanks so much for the fix, clear summary and updated tests.

I'll cut a new major release of Metro afterwards, as this is strictly speaking a breaking change.

sharmilajesupaul · 2021-11-10T19:29:19Z

Thanks @motiz88 & @raejin! Let me know if you'd liked to see more changes made here as well.

packages/metro-resolver/src/resolve.js

Co-authored-by: Rae Liu <raejin@users.noreply.github.com>

facebook-github-bot · 2021-11-11T09:52:08Z

@GijsWeterings has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-11-11T18:14:40Z

@GijsWeterings merged this pull request in 9bbe219.

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

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Nov 10, 2021

raejin reviewed Nov 10, 2021

View reviewed changes

packages/metro-resolver/src/resolve.js Show resolved Hide resolved

Add a comment explaining a changed line

2282b7f

Co-authored-by: Rae Liu <raejin@users.noreply.github.com>

facebook-github-bot closed this in 9bbe219 Nov 11, 2021

facebook-github-bot added the Merged label Nov 11, 2021

Kudo mentioned this pull request Dec 23, 2021

[apps] Enforce 'util' version to fix errors on import expo/expo#15698

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[metro-resolver] Respect transitive deps when using `nodeModulesPaths` #738

[metro-resolver] Respect transitive deps when using `nodeModulesPaths` #738

sharmilajesupaul commented Nov 10, 2021 •

edited

Loading

facebook-github-bot commented Nov 10, 2021

facebook-github-bot commented Nov 10, 2021

facebook-github-bot commented Nov 10, 2021

raejin left a comment

raejin Nov 10, 2021

raejin commented Nov 10, 2021

motiz88 commented Nov 10, 2021

sharmilajesupaul commented Nov 10, 2021

facebook-github-bot commented Nov 11, 2021

facebook-github-bot commented Nov 11, 2021

	const allDirPaths = nodeModulesPaths
	.map(nodeModulePath => path.join(nodeModulePath, realModuleName))
	.concat(extraPaths);

[metro-resolver] Respect transitive deps when using nodeModulesPaths #738

[metro-resolver] Respect transitive deps when using nodeModulesPaths #738

Conversation

sharmilajesupaul commented Nov 10, 2021 • edited Loading

facebook-github-bot commented Nov 10, 2021

Action Required

Process

facebook-github-bot commented Nov 10, 2021

facebook-github-bot commented Nov 10, 2021

raejin left a comment

Choose a reason for hiding this comment

raejin Nov 10, 2021

Choose a reason for hiding this comment

raejin commented Nov 10, 2021

motiz88 commented Nov 10, 2021

sharmilajesupaul commented Nov 10, 2021

facebook-github-bot commented Nov 11, 2021

facebook-github-bot commented Nov 11, 2021

[metro-resolver] Respect transitive deps when using `nodeModulesPaths` #738

[metro-resolver] Respect transitive deps when using `nodeModulesPaths` #738

sharmilajesupaul commented Nov 10, 2021 •

edited

Loading