Metro 0.25+ is resolving package.json within react-native/.../__fixtures__ directory

[jamesreggio]

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

I have an actual dependency upon react-native-vector-icons in my React Native project.

(I'm on RN 0.52.0, but I've manually upgraded to metro@0.26.0 and metro-core@0.26.0 using yarn's resolutions feature.)

React Native includes a package.json for react-native-vector-icons as a test fixture here: https://github.com/facebook/react-native/blob/master/local-cli/core/__fixtures__/files/package.json

(It's been there for a long while.)

In Metro 0.25.1 onwards, the packager will resolve the fixture version of react-native-vector-icons, instead of resolving the actual package at the root of my node_modules. Because the fixture doesn't contain the full source, the packager spits out this error:

error: bundling failed: Error: While trying to resolve module `react-native-vector-icons` from file `/Users/jamesreggio/src/banter/mobile/js/components/controls/core/Icon.js`, the package `/Users/jamesreggio/src/banter/mobile/node_modules/react-native/loc
al-cli/core/__fixtures__/files/package.json` was successfully found. However, this package itself specifies a `main` module field that could not be resolved (`/Users/jamesreggio/src/banter/mobile/node_modules/react-native/local-cli/core/__fixtures__/files
/index.js`. Indeed, none of these files exist:

  * `/Users/jamesreggio/src/banter/mobile/node_modules/react-native/local-cli/core/__fixtures__/files/index.js(.native||.ios.js|.native.js|.js|.ios.json|.native.json|.json)`
  * `/Users/jamesreggio/src/banter/mobile/node_modules/react-native/local-cli/core/__fixtures__/files/index.js/index(.native||.ios.js|.native.js|.js|.ios.json|.native.json|.json)`
    at ResolutionRequest.resolveDependency (/Users/jamesreggio/src/banter/mobile/node_modules/metro/src/node-haste/DependencyGraph/ResolutionRequest.js:108:15)
    at DependencyGraph.resolveDependency (/Users/jamesreggio/src/banter/mobile/node_modules/metro/src/node-haste/DependencyGraph.js:270:4419)
    at /Users/jamesreggio/src/banter/mobile/node_modules/metro/src/DeltaBundler/traverseDependencies.js:201:36
    at Generator.next (<anonymous>)
    at step (/Users/jamesreggio/src/banter/mobile/node_modules/metro/src/DeltaBundler/traverseDependencies.js:256:306)
    at /Users/jamesreggio/src/banter/mobile/node_modules/metro/src/DeltaBundler/traverseDependencies.js:256:536
    at new Promise (<anonymous>)
    at /Users/jamesreggio/src/banter/mobile/node_modules/metro/src/DeltaBundler/traverseDependencies.js:256:217
    at addDependency (/Users/jamesreggio/src/banter/mobile/node_modules/metro/src/DeltaBundler/traverseDependencies.js:256:92)
    at /Users/jamesreggio/src/banter/mobile/node_modules/metro/src/DeltaBundler/traverseDependencies.js:237:9

What is the expected behavior?

I would expect Metro to resolve to the node_modules/react-native-vector-icons/package.json first, and thus silently succeed in bundling.

Please provide your exact Metro configuration and mention your Metro, node, yarn/npm version and operating system.

I'm on RN 0.52.0, but I've manually upgraded to metro@0.26.0 and metro-core@0.26.0 using yarn's resolutions feature.

yarn 1.3.2
Mac OS X 10.12.6 (High Sierra)

[jeanlauliac]

Yup, this is related to changes I made recently!

In Metro 0.25.1 onwards, the packager will resolve the fixture version of react-native-vector-icons, instead of resolving the actual package at the root of my node_modules. Because the fixture doesn't contain the full source, the packager spits out this error:

So, what was happening before is that Metro was already resolving to the fixture version first. However, it would try to resolve the main field, that would fail, and would then proceed to try with the next best candidate, the one in the node_modules.

With the last version of Metro, for the sake of correctness, Metro immediately fails if a main field cannot be resolved, because it means a package is invalid. Here's a good reason to do that: if someday, we'd have added a valid main file in that fixture, then your project would have been incorrectly bundling as well, but silently! Indeed it would have included code from the fixture instead of the real module, and likely crash at runtime instead.

So I believe crashing early to prevent that is the best solution. The reason Metro resolves the fixture first, instead of the node_modules, is because it tries to resolve Haste modules and packages before running the classic Node.js resolution algorithm. With the "Haste" system, packages are basically global and can be required from anywhere, that is used in React Native mostly for historical reasons.

To mitigate the issue, could you try adding the fixtures folder to the blacklist? More specifically, there is a configuration function called getBlacklistRE that can be set in your project's rn-cli.config.js file. For example, this is how that could look like to ignore all the __fixtures__ folders:

module.exports = {
  getBlacklistRE() {
    return /.*\/__fixtures__\/.*/;
  },
};

Going forward, I agree we need to find a reliable long-term solution for this problem. I'm considering whether we could disable Haste package resolution by default, or adding these fixtures to the default list of ignored folders.

[jamesreggio]

Thanks for the thoughtful reply, @jeanlauliac. I can understand the justification for throwing these errors today — in fact, it would be a very challenging regression to debug if old sources to react-native-vector-icons were to be committed to that __fixtures__ directory.

The getBlacklistRE change is an effective workaround, but either RN or Metro will need to find a suitable zero-configuration resolution before this ships.

It's a bit upsetting that Haste resolution must be enabled in the first place (since RN is the only package in most non-FB RN projects that requires Haste resolution), but switching RN internals to use Node's resolutions algorithm would be too large a fish to fry.

It also saddens me to work around this problem by adding more 'magic' to the system (i.e., adding a default blacklist regex that excludes directories based upon Jest's conventions). I feel like you're most likely to take this approach, though, so may I request that you add a config value like excludeJestFixtures and default it to true instead of using the existing getBlacklistRE mechanism? I'm afraid that if you set a default getBlacklistRE to exclude __fixtures__, anybody who manually sets a getBlacklistRE would need to know to include the __fixtures__ exclusion; otherwise, they'd see new errors crop up from setting it.

Finally, forgive me if this stream of consciousness isn't adequately clear. It's a very chatty day in our open floorplan office :(

[jeanlauliac]

The getBlacklistRE change is an effective workaround, but either RN or Metro will need to find a suitable zero-configuration resolution before this ships.

Agreed, we need to find a robust solution. One solution I'm thinking about might be to enable Haste only for very specific directories, and ignore a number of files by default. I'll look into that.

It's a bit upsetting that Haste resolution must be enabled in the first place (since RN is the only package in most non-FB RN projects that requires Haste resolution), but switching RN internals to use Node's resolutions algorithm would be too large a fish to fry.

Yeah, I dislike Haste in general, as it makes everything global and hard to track. The ideal solution would be to not have Haste by default, but you're right switching RN to not use Haste is probably too much hassle for now.

It also saddens me to work around this problem by adding more 'magic' to the system (i.e., adding a default blacklist regex that excludes directories based upon Jest's conventions). I feel like you're most likely to take this approach, though, so may I request that you add a config value like excludeJestFixtures and default it to true instead of using the existing getBlacklistRE mechanism? I'm afraid that if you set a default getBlacklistRE to exclude fixtures, anybody who manually sets a getBlacklistRE would need to know to include the fixtures exclusion; otherwise, they'd see new errors crop up from setting it.

Whatever is returned from getBlacklistRE would be combined with the default ignore yeah, if we go that way.

[jeanlauliac]

Well, this exploded:

• Presence of local-cli __fixtures__ file breaks react-native-vector-icons react-native#17610
• Error: While resolving module react-native-vector-icons/MaterialIcons, the Haste package react-native-vector-icons was found. oblador/react-native-vector-icons#626
• the Haste package 'react-native-vector-icons' oblador/react-native-vector-icons#630

And many more. @jamesreggio it appears it's been mitigated already in facebook/react-native#17672, already in RN's master. But we'll keep tracking this and try to find a better solution long term. I'll also follow up on improving Metro's error messages.

[jamesreggio]

Ugh, yeah. Thanks for calling out the wrong answer here.

The fact that Metro already has a builtin blacklist with knowledge of other Facebook-sponsored packages leads me to think that the right place for a hacky workaround is still on the Metro side.

I like your idea of limiting Haste resolution to specific FB-sponsored packages. It feels like it will generate the least surprise in the long run. Thanks for thinking about this.

[ericketts]

sounds like this will break the nasty hack I'm using to get absolute imports working