Fix providesModuleNodeModules behavior to be more deterministic. #33

skevy · 2016-02-18T03:13:48Z

--- Relates to facebook/react-native#5985 ---

Currently, the providesModuleNodeModules option allows for specifying an array of package names, that the packager will look for within node_modules, no matter how deep they're nested, and treat them as packages that use the @providesModule pragma.

In reality, this is not actually the behavior we want.

npm, because of how it handles dependencies, can do all kinds of arbitrary nesting of packages when npm install is run. This causes a problem for how we deal with providesModuleNodeModules. Specifically...take fbjs. Currently, React Native relies on using the Haste version of fbjs (will be resolved in #5084). Thus if npm installs multiple copies of fbjs...which is a very common thing for it to do (as can be seen by this list of issues: https://github.com/facebook/react-native/issues?utf8=%E2%9C%93&q=naming+collision+detected), we get into a state where the packager fails and says that we have a naming collision.

Really, the behavior we want is for the packager to treat only a single copy of a given package, that we specify in the Resolver in the providesModuleNodeModules option, as the package that it uses when trying to resolve Haste modules.

This PR provides that behavior, by changing providesModuleNodeModules from a list of strings to a list of objects that have a name key, specifying the package name, as well as a parent key. If parent is null, it will look for the package at the top level (directly under node_modules). If parent is specified, it will use the package that is nested under that parent as the Haste module.

To anyone who reads this PR and is familiar with the differences between npm2 and npm3 -- this solution works under both, given the fact that we are now shipping the NPM Shrinkwrap file with React Native when it's installed through npm. In both the npm2 and npm3 case, node_modules specified by RN's package.json are nested underneath react-native in node_modules, thus allowing us to specify, for example, that we want to use React Native's copy of fbjs (not any other copy that may be installed) as the module used by the packager to resolve any requires that reference a module in fbjs.

skevy · 2016-02-18T03:14:00Z

cc @cpojer @martinbigio

cpojer · 2016-02-18T03:15:10Z

NICE

cpojer · 2016-02-18T04:17:41Z

src/DependencyGraph/DependencyGraphHelpers.js

+   * or examples).
+   */
+  _resolveHastePackages(packages) {
+    const _packagePathForPackage = ({ name, parent }, rootDir) => {


can you drop the leading underscore? :)

cpojer · 2016-02-18T05:09:42Z

src/DependencyGraph/DependencyGraphHelpers.js

+        packagePath = path.join(rootDir, 'node_modules', name);
+      }
+
+      if (packagePath[packagePath.length - 1] === path.sep) {


I think .endsWith here would be fine too :P

If I read this correctly, this can only ever be true if name ends with path.sep. Does that happen or is this defensive programming? Maybe it’s better sanitizing at the top of the function? Does it matter at all?

This can happen in the case of a providesModuleNodeModule entry that looks like:

{ name: 'some-module', parent: ['parentPackage1', 'parentPackage2'] }, as the path will end with /node_modules/

cpojer · 2016-02-18T05:11:20Z

Alright, looks great to me. I'd be happy to merge. I think it makes sense to add one or two tests for DependencyGraphHelpers so that we can make sure this behavior will continue to work when we touch this code again?

davidaurelio · 2016-02-18T08:46:14Z

src/DependencyGraph/DependencyGraphHelpers.js

  isNodeModulesDir(file) {
-    const index = file.lastIndexOf('/node_modules/');
+    const index = file.indexOf('/node_modules/');


file.indexOf(${path.sep}node_modules${path.sep});? that’s so ugly that you might want to put a constant at the top of the file :-)

davidaurelio · 2016-02-18T08:55:22Z

Thank you for this PR!
Will this also work for cases like

project-dir
└ node_modules
  ├ react
  │ └ node_modules
  │   └ [email protected]
  └ react-native
    └ node_modules
      └ [email protected]

?

Add some meaningful tests, please, and make sure you actually see them fail once without your code changes.

davidaurelio · 2016-02-18T09:01:46Z

As @cpojer mentioned, this code has some hefty micro optimizations, especially around the path module.

consider turning these:

parent.join(path.sep + 'node_modules' + path.sep)
// …
packagePath = path.join(rootDir, 'node_modules', name);
// …
const index = file.indexOf('/node_modules/');

into

const NODE_MODULES = path.sep + 'node_modules' + path.sep;
// …
parent.join(NODE_MODULES)
// …
packagePath = rootDir + NODE_MODULES + name;
// …
const index = file.indexOf(NODE_MODULES);

skevy · 2016-02-18T09:07:36Z

@davidaurelio yep that case is exactly what this PR is meant to address -- as I say in my PR description, the issue with how this was being handled before was that the providesModuleNodeModules option wasn't deterministic enough to guarantee that we were looking for the correct version of a package that was listed (such as fbjs)...it would just treat all of versions that it found as Haste packages, resulting in duplicate errors. This PR resolves that.

Per both you and @cpojer's comments, I will add a few tests (and will make sure they're valid tests haha).

Also, I've already noticed the perf issue and am in the process of profiling the new impl of this method against the old one, and am determined to at least make it match the old perf (if not better it slightly).

skevy · 2016-02-18T09:09:37Z

I spoke to @cpojer offline about what I'm working on re this PR as well...if it weren't for silly time zones it probably would have been done before you even got a chance to do your review of this :-p

Thanks for your comments :-)

skevy · 2016-02-19T00:52:04Z

Just updated stuff @davidaurelio @cpojer.

I made the fixes requested in both of your comments and also added tests (both for DependencyGraphHelpers directly as well as another test in DependencyGraph-tests...which I confirmed was broken with the current code in master).

In addition, I was able to make a pretty significant perf improvement.

Instead of looping through and doing string comparisons in isNodeModulesDir, I instead precompile a regex that allows for testing all providesModuleNodeModules at once.

I'm able to get a > 90% perf improvement on that method using the regex technique. I also confirmed that this perf improvement exists on both Node 4 and 5.

cpojer · 2016-02-19T02:26:04Z

src/DependencyGraph/DependencyGraphHelpers.js

-      if (parts.indexOf(dirs[i]) > -1) {
-        return false;
-      }
+    if (this._hasteRegex.test(file)) {


return !this._hasteRegex.test(file); ?

Yes. Definitely. My bad.

cpojer · 2016-02-19T02:30:02Z

Nice, thanks for the work and for making the perf better :) I'll defer to @davidaurelio to take another look and merge it.

skevy · 2016-02-19T02:45:18Z

Ok, got rid of the .reduce's @cpojer for readability's sake.

davidaurelio · 2016-02-20T09:58:20Z

@skevy thank you for taking the time to wrap this up. Lgtm. The leading underscore convention for module-private functions looks a little awkward to me, but I can live with it.

I’m just. figuring out the best order to merge all open PRs. Patch-level stuff first. Given that this changes the format of providesModuleNodeModules, it will be v3.0.0.

Your patch has conflicts.

cpojer · 2016-02-25T08:30:50Z

@skevy are you rebasing this so @davidaurelio can merge it?

skevy · 2016-02-26T18:05:09Z

@davidaurelio @cpojer sorry for the delay on this. was traveling. Fixed conflicts and got rid of that underscore in the module private function. Should be good to go.

Currently, the `providesModuleNodeModules` option allows for specifying an array of package names, that the packager will look for within node_modules, no matter how deep they're nested, and treat them as packages that use the `@providesModule` pragma. In reality, this is not actually the behavior we want. npm, because of how it handles dependencies, can do all kinds of arbitrary nesting of packages when `npm install` is run. This causes a problem for how we deal with `providesModuleNodeModules`. Specifically...take `fbjs`. Currently, React Native relies on using the Haste version of `fbjs` (will be resolved in #5084). Thus if npm installs multiple copies of fbjs...which is a very common thing for it to do (as can be seen by this list of issues: https://github.com/facebook/react-native/issues?utf8=%E2%9C%93&q=naming+collision+detected), we get into a state where the packager fails and says that we have a naming collision. Really, the behavior we want is for the packager to treat only a *single* copy of a given package, that we specify in the `Resolver` in the `providesModuleNodeModules` option, as the package that it uses when trying to resolve Haste modules. This PR provides that behavior, by changing `providesModuleNodeModules` from a list of strings to a list of objects that have a `name` key, specifying the package name, as well as a `parent` key. If `parent` is null, it will look for the package at the top level (directly under `node_modules`). If `parent` is specified, it will use the package that is nested under that parent as the Haste module. To anyone who reads this PR and is familiar with the differences between npm2 and npm3 -- this solution works under both, given the fact that we are now shipping the NPM Shrinkwrap file with React Native when it's installed through `npm`. In both the npm2 and npm3 case, node_modules specified by RN's package.json are nested underneath `react-native` in node_modules, thus allowing us to specify, for example, that we want to use React Native's copy of `fbjs` (not any other copy that may be installed) as the module used by the packager to resolve any `requires` that reference a module in `fbjs`.

Fix providesModuleNodeModules behavior to be more deterministic.

davidaurelio · 2016-03-05T14:17:54Z

Published as v2.6.0

cpojer · 2016-03-06T04:12:02Z

This PR completely breaks Jest 0.9 unfortunately. We need to revert and re-publish or fix this ASAP (and the next version of Jest should lock node-haste to ~ instead of ^). Pretty sure the same thing will come up in react-native too.

See jestjs/jest#771 – the git repo included ( https://github.com/AdamKyle/Toy-Box ) shows well how it breaks when you remove everything inside of modulePathIgnorePatterns.

This reverts commit 8100c8d, reversing changes made to b0b5044.

facebook-github-bot added the CLA Signed label Feb 18, 2016

skevy mentioned this pull request Feb 18, 2016

Make providesModuleNodeModules behavior to be more deterministic. facebook/react-native#5985

Closed

cpojer reviewed Feb 18, 2016
View reviewed changes

skevy force-pushed the fix-providesModuleNodeModules branch 2 times, most recently from 9661c67 to ed4669e Compare February 18, 2016 04:59

cpojer reviewed Feb 18, 2016
View reviewed changes

davidaurelio reviewed Feb 18, 2016
View reviewed changes

skevy force-pushed the fix-providesModuleNodeModules branch from 4f0118f to 4a8458c Compare February 19, 2016 01:52

cpojer reviewed Feb 19, 2016
View reviewed changes

skevy force-pushed the fix-providesModuleNodeModules branch from 4a8458c to 6e092bb Compare February 19, 2016 02:44

mkonicek mentioned this pull request Feb 19, 2016

Merge PRs that get master back to good state facebook/react-native#6037

Closed

skevy force-pushed the fix-providesModuleNodeModules branch from 6e092bb to 081ee58 Compare February 19, 2016 19:58

skevy mentioned this pull request Feb 19, 2016

Update providesModuleNodeModules option to use new format. facebook/react-native#6039

Closed

skevy force-pushed the fix-providesModuleNodeModules branch 2 times, most recently from c97b296 to 91520b6 Compare February 26, 2016 18:04

skevy mentioned this pull request Feb 26, 2016

Duplicate haste modules because of npm shrinkwrap facebook/react-native#6161

Closed

skevy force-pushed the fix-providesModuleNodeModules branch 2 times, most recently from efd561b to 7cdf088 Compare February 26, 2016 19:38

mkonicek mentioned this pull request Mar 3, 2016

Move react to peerDependencies facebook/react-native#5813

Closed

skevy force-pushed the fix-providesModuleNodeModules branch from 7cdf088 to fc3cf7f Compare March 4, 2016 05:55

skevy force-pushed the fix-providesModuleNodeModules branch from fc3cf7f to 7707b17 Compare March 4, 2016 05:56

davidaurelio added a commit that referenced this pull request Mar 5, 2016

Merge pull request #33 from skevy/fix-providesModuleNodeModules

8100c8d

Fix providesModuleNodeModules behavior to be more deterministic.

davidaurelio merged commit 8100c8d into facebookarchive:master Mar 5, 2016

mkonicek mentioned this pull request Mar 7, 2016

Attempt to make RN work with npm 2 facebook/react-native#6343

Closed

davidaurelio added a commit that referenced this pull request Mar 9, 2016

Revert "Merge pull request #33 from skevy/fix-providesModuleNodeModules"

dc60fa8

This reverts commit 8100c8d, reversing changes made to b0b5044.

Fix providesModuleNodeModules behavior to be more deterministic. #33

Fix providesModuleNodeModules behavior to be more deterministic. #33

Uh oh!

Conversation

skevy commented Feb 18, 2016

Uh oh!

skevy commented Feb 18, 2016

Uh oh!

cpojer commented Feb 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpojer commented Feb 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidaurelio commented Feb 18, 2016

Uh oh!

davidaurelio commented Feb 18, 2016

Uh oh!

skevy commented Feb 18, 2016

Uh oh!

skevy commented Feb 18, 2016

Uh oh!

skevy commented Feb 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpojer commented Feb 19, 2016

Uh oh!

skevy commented Feb 19, 2016

Uh oh!

davidaurelio commented Feb 20, 2016

Uh oh!

cpojer commented Feb 25, 2016

Uh oh!

skevy commented Feb 26, 2016

Uh oh!

davidaurelio commented Mar 5, 2016

Uh oh!

cpojer commented Mar 6, 2016

Uh oh!

Uh oh!