Stop using npm link in tests

[Timer]

npm link is super unreliable and causes our tests to fail; this replaces npm link with a much more predictable behavior.

This doesn't work on AppVeyor yet because the version of rsync is complaining about a local copy; any ideas?

[gaearon]

Can we just use cp -r? Why do we need rsync?

[Timer]

We want to skip node_modules; we could cp -r & rm -r, then install because only want dependencies and not dev ones.

[gaearon]

Sounds reasonable

[Timer]

This should be good to land.

[gaearon]

Can you explain what this does and why?

[Timer]

Usages of npm link were swapped out with install_package, which behaves like so:

1. Clear existing instances of the package (rm -rf **/pkg/)
   • why: The package may exist at arbitrarily nested levels in the package tree, so we want to ensure it's removed everywhere. We should be testing our source controlled version of the package, not one grabbed from npm. This will ensure any request for our package drops into node module resolution and searches up the tree for the copy of the package we're about to create.
2. The package is copied into the root-most level node_modules to ensure it can be resolved by all packages which require it (cp -R ${1%/} node_modules/).
   • This matches behavior that would be seen in npm >= 3 & Yarn (package hoisting).
3. Remove all node_modules which were copied over with the package (rm -rf node_modules/$pkg/node_modules/).
   • why: These packages may include development dependencies, which we do not want to include while testing (we want to be as close to a real install as possible).
4. Production dependencies are installed in the package we are simulating a link for (cd node_modules/$pkg/ && npm install --only=production).
   • why: We need them 😄
   • Sure, these packages aren't hoisted but we're testing our functionality, not the correctness of the package manager -- we're sort of reverting to npm 2 behavior [which was better for testing anyways].
5. We remove any installed dependencies of the linked package which are in our monorepo -- this ensures Node module resolution kicks in and we search up a level where the package should also of been linked (rm -rf node_modules/{babel-preset-react-app,eslint-config-react-app,react-dev-utils,react-error-overlay,react-scripts}).

[gaearon]

Why do we need to run Yarn after ejecting?

[Timer]

The dependency tree is invalid after ejecting since we don't run Yarn, so the installation is broken.
This happens in existing applications and now in our CI since we use a more "real life" install_package instead of npm link, which was masking this failure.

Note babel-loader is installed under react-scripts and not top level.

$ npx create-react-app testing # installs via Yarn
$ npm ls babel-loader
[email protected] /Users/joe/Desktop/testing
└─┬ [email protected]
  └── [email protected] 
$ echo y | yarn eject
$ npm ls babel-loader
[email protected] /Users/joe/Desktop/testing
└── UNMET DEPENDENCY [email protected]
$ yarn build
yarn run v1.2.1
$ node scripts/build.js
module.js:529
    throw err;
    ^

Error: Cannot find module 'babel-loader'
...
$ yarn
yarn install v1.2.1
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
[4/4] 📃  Building fresh packages...
success Saved lockfile.
✨  Done in 3.43s.
$ yarn build
yarn run v1.2.1
$ node scripts/build.js
Creating an optimized production build...
Compiled successfully.
...

See #3347

[gaearon]

Should we just fix our eject code to do npm uninstall --save react-scripts and then npm install --save <its dependencies>, and do the same for Yarn (yarn remove react-scripts and yarn add <its dependencies>)? Then we won't need to do CI-specific hacks and will solve the problem of incorrect lockfile.

[Timer]

We could make that adjustment, but I'm not sure it's necessary -- Yarn fixes the invalid lockfile as of July (the react-scripts entry is removed).
I'm not sure about npm's package-lock.json.

The CI-specific hack isn't so IMO, because our end-users have to do this. The project is broken otherwise.

[gaearon]

It might not be necessary but it feels more consistent and bringing less surprises in the future.

[gaearon]

... assuming we figure out how to run npm in the right folder 😉

[Timer]

Want to land this as-is and make the eject changes in a separate PR? I can do it in this PR if you'd like.

[gaearon]

Yep, sounds good. Thanks.