Skip to content

Conversation

@cdebost
Copy link
Collaborator

@cdebost cdebost commented Jan 31, 2019

This is a partial application of #117 including only the changes relevant to npm 3+ support. #117 and its sister PRs in montage and mop introduced too many changes to review all at once.

This PR introduces no breaking changes for montage#master, aside from the non-harmful warnings (see "Impact").

Background

In npm 3+, dependencies will be installed as close to the root as possible without causing version conflicts, meaning a module may find its dependency under its own node_modules, or the parent's node_modules, or the grandparent's node_modules, etc. This is different from npm 2 where dependencies-of-dependencies are always installed under the package's own node_modules.

The fix to support npm 3+ is two-fold:

  1. Under npm 5+, a package-lock.json file is generated, which encodes the exact structure of node_modules. When un-mopped, mr will attempt to load a package-lock.json file under the application root and use it to resolve dependencies.
  2. If no package-lock.json is available, dependencies will be retried on all parent directories' node_modules.

Impact

When running mopped apps, nothing will change regardless of the node/npm version.

When running un-mopped:

  • npm 2 (node 4): Non-harmful 404 errors will appear in the browser console as mr retries bluebird.
  • npm 3 (node 6), or npm 5+ and package-lock disabled: The app will load, but the browser console will fill up with non-harmful 404 errors as mr retries dependencies at different locations.
  • npm 5+ (node 8+): The app will run normally, with no new 404 errors in the browser.

@cdebost cdebost force-pushed the feature/npm3 branch 5 times, most recently from be15f63 to 7f91b86 Compare January 31, 2019 20:43
@cdebost cdebost mentioned this pull request Jan 31, 2019
@cdebost cdebost requested review from marchant and tejaede February 1, 2019 03:18
Copy link
Member

@marchant marchant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, I tested with multiple applications and through an additional pull request depending on this.

@marchant
Copy link
Member

marchant commented Aug 9, 2019

I believe I independently fixed the exception happening in the mop integration in a PR that depends on this. Merging.

@marchant marchant merged commit 6555ba1 into master Aug 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants