NPM modules being preferred over Haste modules

[mikelambert]

I've debugged facebook/react-native#13765 (comment) down to an issue where RN's var merge = require('merge') will import node_modules/merge, instead of the expected react-native/Libraries/vendor/core/merge.js (which contains a @providesModule merge directive).

According to the dozens of users affected, it seems to be non-deterministic behavior that sometimes resolves itself with reinstallation, different versions of node, or recreation of the directory and node_modules directory. I'm not yet sure if this nondeterminisim is a bug in React-Native library code layout, or a bug in the RN packager (aka metro-bundler?) itself...but I'm leaning towards the latter, since it seems to violate my expectations for Haste module imports.

[jeanlauliac]

I think Haste modules are always preferred, but it's possible it is a problem in jest-haste-map that make it not detect the existence of the merge Haste module as expected in particular conditions. The resolution logic wasn't changed so I'm surprised it appears to be a recent regression. We did add some bug fixes to jest-haste-map, possibly one could have caused a regression.

[mikelambert]

So another few hours of debugging, and I found that resetting the cache fixes things. Posted a follow-up with my analysis of why various voodoo magic fixes worked: facebook/react-native#13765 (comment)

No idea what led to a corrupted cache in the first place. I have my old "busted" cache, if you are interested still.

The relevant merge bit, which explains why it was skipping the haste resolution and falling back on node module resolution:

"mergeInto":{"g":["/fullpath/mobile/node_modules/react-native/Libraries/vendor/core/mergeInto.js",0]},
"mergeHelpers":{"g":["/fullpath/mobile/node_modules/react-native/Libraries/vendor/core/mergeHelpers.js",0]},
"merge":{}

And the "duplicates" property, which included:

"merge":{"g":{
  "/fullpath/mobile/node_modules/react-native/node_modules/merge/package.json":1,
  "/fullpath/mobile/node_modules/react-native/Libraries/vendor/core/merge.js":0
}}

[jeanlauliac]

Ohhh! This is very useful, thank you. Unfortunately, I expect thing to get broken again. This is actually a legitimate problem, and it needs to generate an error more proeminently. The duplication of modules stored in the duplicates field is real. I think there are 2 action items from there:

1. we need to make the HasteMap#getModule function crash immediately if there is module duplication for the requested module name. Right now, it's silent, so callsites believe the module is just not there. This is incorrect.
2. we may want to ignore "haste packages" contained inside node_modules, and/or the duplication detection system should keep packages and modules separate, that it doesn't look like to do right now.

[mikelambert]

Glad to hear it helped!

FWIW, I believe this duplicate module merge comes directly from react-native->sane->exec-sh->merge dependency, so is unrelated to anything I've imported. So you fixing # 1 alone would probably break RN.

Unfortunately, judging from the bug that led me here, I'm not sure anyone on the RN side is aware of this duplicate module issue yet.

[ide]

we may want to ignore "haste packages" contained inside node_modules

Last time I looked into this for RN 0.45, I think I arrived at a similar conclusion. In RN, providesModuleNodeModules whitelists only react-native now, but IIRC the whitelist includes <project>/node_modules/react-native/**, which includes <project>/node_modules/react-native/node_modules/**. I think it'd be enough to tell jest-haste-map to ignore the node_modules subdirectory under the whitelisted modules.

[mikelambert]

In my case, I believe merge was adjacent to react-native in my top-level node_modules, not within react-native/node_modules/.

Not sure if I was doing something wrong to achieve that end-result? In case it's relevant, I was building react-native myself, and then npm install ../path-to/react-native into my mobile app's node_modules/.

[ide]

Could you perhaps have had multiple copies of merge under <proj>/node_modules/react-native/?

[ide]

That's what it looks like from the last JSON snippet you posted:

"merge":{"g":{
  "/fullpath/mobile/node_modules/react-native/node_modules/merge/package.json":1,
  "/fullpath/mobile/node_modules/react-native/Libraries/vendor/core/merge.js":0
}}

I think if we filter out node_modules/react-native/node_modules we'd go down to just node_modules/react-native/Libraries/vendor/core/merge.js as desired.

[mikelambert]

Ahhhh really?! Thanks for double-checking my assumptions! I was speaking on what exists in my directory now...which I mistakenly assumed was what existed back when things were broken. :(

Man, talk about a merge conflict...hah!

So perhaps my "fix" was to have removed react-native/node_modules/merge/, but I needed to rebuild the watchman cache for anything to have noticed that and start working again. And I mistakenly assumed it was rebuilding the cache that made things work for me?

(going to update my "fix instructions" comment facebook/react-native#13765 in the react-native issue to reflect this)

[jeanlauliac]

I make a change to jest-haste-map that will make it throw if duplicate modules are found: jestjs/jest#3976. This doesn't fix this problem of duplicated merge modules, but this will make it much more explicit what's going on, that I think it's the correct approach. The merge module that is in node_modules will have to be blacklisted from jest-haste-map.

[ide]

@jeanlauliac I think this PR might fix things (filters out nested node_modules). Could you take a look at it? jestjs/jest#3984

[ide]

The jest-haste-map PR landed (thank you @jeanlauliac!) so sometime in the future when it gets published to npm we'll be able to use it in Metro (and indirectly, RN). Fingers crossed that the duplicate module collisions are gone forever unless there are actually duplicates within a package's own code.

[jeanlauliac]

Do you think we should we release a new minor/rev version off the last major to fix the issues people have right now?

[ide]

@jeanlauliac That's a nice idea, why don't we try it if it's not too much trouble to publish jest-haste-map to npm? RN 0.46 uses a version of Metro that uses jest-haste-map ^20.0.1 so people should automatically be able to get minor & patch updates. (Perhaps we should first set the latest npm dist-tag to an older version so that people only get the new jest-haste-map if they explicitly install it, so people like me and mikelambert can test it out.)

[SimenB]

There is an jest-haste-map@20.1.0-beta.1 on npm if you want to test some stuff out?

[jeanlauliac]

I released 20.0.5 with jestjs/jest#3984, do you want to try that out, make sure everything is in order? I kept the latest tag on 20.0.4, but I think this won't prevent people from getting the new version if they have the ^20.0.1 specifier. Since tests pass, and that we've been having the beta/chi versions internally for some days, I trust this shouldn't cause problems. At worst we can rollback and release 20.0.6.

[ide]

jest-haste-map 20.0.5 appears to be working fine in several of our projects including a test suite.

[jeanlauliac]

Looks like we're good to close. Feel free to reopen if you still observe problems!