fix: always install leaves using npm --global-style

[ricky]

Description

Use npm install --global-style under lerna bootstrap --hoist for packages which cannot be hoisted. This results in more consistently correct behavior at the cost of increased duplication of some packages.

Motivation and Context

In the event that an explicit dependency (dep1@X) of a package is hoisted but other dependencies with a common sub-dependency on a different version of dep1@Y cannot be hoisted, npm will attempt to share that package by installing dep1@Y in the root of the package's node_modules directory. As a result, the incorrect version of dep1 will be available to the package rather than the hoisted dependency it requires.

Scenario

1. A package has three explicit dependencies: [email protected], dep2, and dep3.
2. dep2 & dep3 have a transitive dependency on a different version of dep1 ([email protected])
3. [email protected] can be hoisted, but dep2 & dep3 cannot (either due to version mismatches or explict --nohoist directives).

Current Result

├── node_modules
│   └── [email protected]
└── my-package
    └── node-modules
        ├── [email protected]
        ├── dep2
        └── dep3

Desired result

├── node_modules
│   └── [email protected]
└── my-package
    └── node-modules
        ├── dep2
            └── [email protected]
        └── dep3
            └── [email protected]

Result without hoisting

├── my-package
    └── node-modules
        ├── [email protected]
        ├── dep2
            └── [email protected]
        └── dep3
            └── [email protected]

See: #677

How Has This Been Tested?

Basic test project and manual validation steps outlined here:
https://gist.github.com/ricky/ff42dd94d52f2c3e25218d6dcb0a5372

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

From the perspective of npm users, this probably qualifies as a non-breaking change. For yarn users, the client override would constitute a change in functionality.

Checklist:

• My code follows the code style of this project. (AFAICT)
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

I had some difficulty crafting a unit test to cover this case. With the way the mocks & fixtures are set up, it's not immediately clear how to represent the transitive dependency of an external dependency.

[evocateur]

@ricky This looks great so far, thanks! A couple more simple tests and we're good to go.

[evocateur]

An NpmUtilities.installInDir() test case with an assertion similar to this one would help keep our coverage up.

const config = { npmClient: "yarn" };
const npmGlobalStyle = true;

NpmUtilities.installInDir(directory, dependencies, config, npmGlobalStyle, (err) => {
  // ...
  expect(ChildProcessUtilities.exec).lastCalledWith("npm", ["install", "--global-style"], {
    // ...

[evocateur]

An assertion in this test would be helpful:

expect(NpmUtilities.installInDir).toBeCalledWith(
  expect.stringContaining("package-dirname"),
  expect.arrayContaining(["non-hoisted-dependency-name"]),
  expect.any(Object),
  true, // npmGlobalStyle
  expect.any(Function)
);

Of course, this means our BootstrapCommand/basic fixture (or a new, hoist-focused variant) needs a non-hoistable external dependency in one of its packages.

[evocateur]

We can backfill the tests later, the gist you made clearly shows that this works as intended.

[ricky]

Thanks. I'll get those test put together by Monday.

[evocateur]

@ricky Awesome!

[jppurcell9]

Nice work @ricky, thanks for the fix!

[lock]

This thread has been automatically locked because there has not been any recent activity after it was closed. Please open a new issue for related bugs.