Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to resolve some dependencies from ‘extraNodeModules’ paths (Windows only) #51

Open
stephen-peck opened this issue Sep 7, 2017 · 1 comment

Comments

@stephen-peck
Copy link

Specifically this affects a subset of dependencies with names containing forward slash, that are either:

  • scoped, e.g. '@myco/mymodule'
  • relative to module e.g. 'mymodule/folder'

... where said modules exist under a path referenced by extraNodeModules

Existing behaviour:

On Windows:

Unable to resolve module `@alexbinary/object-deep-assign` from . . . : Module does not exist in the module map

(working as intended on other platforms)

Root cause:

When resolving extraNodeModules, toModuleName is split using the platform specific path.sep (so backslash on Windows), but would more commonly contain a forward slash regardless of platform.
In the example above the module name will not split on \ and fails to resolve a folder @alexbinary/object-deep-assign.
I would expect the module name to split on either \ or / on Windows, in which case a folder @alexbinary would be resolved correctly.

Possible Fix

On Windows, when we need to split such a path to resolve, we need to consider both separators.

A potential fix at https://github.com/facebook/metro-bundler/blob/14428a67e5e2c9842af4cb864d0f458b3038959d/packages/metro-bundler/src/node-haste/DependencyGraph/ModuleResolution.js#L269
is to use a regex

        const bits = toModuleName.split(path.sep === '/' ? path.sep : /[\/\\]/);`

… I’m happy to to submit this as a pull request.

Environment

The problem was observed in 0.7.8 but is still present in the latest master (above link), although the code has been considerably refactored since
npm 3.10.6, node v6.11.2

Note this is unrelated to other path separator fixes, e.g. for issue #2

@cpojer
Copy link
Contributor

cpojer commented Oct 9, 2017

Yes, please send a PR with tests for this :) Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants