Automatic hoisting of common dependencies #507
Conversation
|
Sweet! Will this be opt-in, or automatic? I recall someone here spiking something like this and having issues, so hoping this wont stop us from upgrading in the future (more hoping it just works and we can have the sweet speed increases) |
|
Sounds great! I guess that this would help a lot with #505. |
|
I tested it for CKEditor 5 and the install time shortened from like 5 minutes to 35 seconds :) |
|
@lukebatchelor It's opt-in with Default behavior is unchanged. The hoist option is boolean with no argument (hoist everything), but it takes an optional glob. There is also a @Reinmar That's awesome! 🚀 |
|
Reviewers please take a look at the original three PRs listed in the description above. There are three distinct change sets here. The main additional enhancement beyond hoisting is exposure of a |
| if (rootVersion) { | ||
| this.logger.warn( | ||
| `"${pkg}" package depends on ${name}@${version}, ` + | ||
| `which differs from the hoisted ${name}@${rootVersion}.` |
There was a problem hiding this comment.
Is this really worth a warning? I can see an argument for warning about devDependencies, but warning for production dependencies seems overwrought.
There was a problem hiding this comment.
It means that something you wanted to have hoisted couldn't be hoisted. That's at best going to slow down your bootstrap, and at worst going to cause errors due to multiple instances of a dependency when you expected one to be shared. This warning has saved us a few times in React Server!
There was a problem hiding this comment.
yes, this warning has helped me out in the past too, especially if you want to have the same dependency version across all packages.
|
|
||
| // Get the version required by the repo root (if any). | ||
| // If the root doesn't have a dependency on this package then we'll | ||
| // install the most common dependency there. |
There was a problem hiding this comment.
Having used this previously with asini, I'm concerned about how hoisting all packages, even ones not specified in the root package.json, all the way to the root makes future npm commands (ls, outdated, shrinkwrap, etc) basically worthless due to "extraneous package" errors.
I am all for recognizing when the root package.json has a matching version already, and doing the appropriate binary linking if necessary. Could we perhaps install the "common among packages but not in root" dependencies in a sibling of the packages themselves? (In the case of multiple packages locations, the closest common ancestor would suffice)
For example, with the default 'packages/*' glob, any common dependencies not present in the root would install into ./packages/node_modules, thus preserving the usefulness of root npm commands while gaining the benefits of hoisting. Does that sound reasonable?
There was a problem hiding this comment.
IMO it only really makes sense to put deps in the leaves or the root. Anywhere else is going to either require special configuration or have weird behavior (or both). Would a lerna clean help with the interference with npm commands at the root?
Actually come to think of it I don't think lerna clean will remove anything at the root. 🤔
There was a problem hiding this comment.
@evocateur i experimented with trying to get commands such as npm outdated to work by symlinking the hoisted deps at the root to the node_modules of each package. this caused lots of issues with the way npm treats symlinks. IMO, it wasn't worth it.
one of the other problems i ran into was forgetting to add a dependency to a package's package.json file because it was hoisted from another package and everything worked as expected. i thought of creating a tool for checking these sorts of things, but never got around to it.
|
I'm willing to forgo `npm outdated` for `packages/node_modules` (which satisfies all common dependencies from `packages/*/package.json`), no fancy re-symlinking necessary. I'm more concerned about `npm shrinkwrap` in the root always failing due to extraneous dependencies.
… On Jan 23, 2017, at 20:32, Ry Racherbaumer ***@***.***> wrote:
@rygine commented on this pull request.
In src/commands/BootstrapCommand.js:
> +
+ // determine where each dependency will be installed
+ Object.keys(depsToInstall).forEach((name) => {
+ const {versions, dependents} = depsToInstall[name];
+
+ let rootVersion;
+
+ if (scope && PackageUtilities.getFilteredPackage({name}, {scope, ignore})) {
+
+ // Get the most common version.
+ const commonVersion = Object.keys(versions)
+ .reduce((a, b) => versions[a] > versions[b] ? a : b);
+
+ // Get the version required by the repo root (if any).
+ // If the root doesn't have a dependency on this package then we'll
+ // install the most common dependency there.
@evocateur i experimented with trying to get commands such as npm outdated to work by symlinking the hoisted deps at the root to the node_modules of each package. this caused lots of issues with the way npm treats symlinks. IMO, it wasn't worth it.
one of the other problems i ran into was forgetting to add a dependency to a package's package.json file because it was hoisted from another package and everything worked as expected. i thought of creating a tool for checking these sorts of things, but never got around to it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
That's fair, as long as we *only* warn.
… On Jan 23, 2017, at 20:33, Ry Racherbaumer ***@***.***> wrote:
@rygine commented on this pull request.
In src/commands/BootstrapCommand.js:
> + : `${name}@${rootVersion}`,
+ });
+ }
+
+ // Add less common versions to package installs.
+ Object.keys(versions).forEach((version) => {
+
+ // Only install deps that can't be hoisted in the leaves.
+ if (version === rootVersion) return;
+
+ dependents[version].forEach((pkg) => {
+
+ if (rootVersion) {
+ this.logger.warn(
+ `"${pkg}" package depends on ${name}@${version}, ` +
+ `which differs from the hoisted ${name}@${rootVersion}.`
yes, this warning has helped me out in the past too, especially if you want to have the same dependency version across all packages.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Updated to apply cleanly to current master. Any 👍 👎 on this? Would be nice to get React Server back onto Lerna. |
There was a problem hiding this comment.
@gigabo one of the things that I have found frustrating about working with React Server is that asini's hoisting makes it impossible to work on a single module as if it were an npm packags. I find using npm or yarn directly on single node modules makes local development much easier for me, and makes lerna repos more accessible for developers who are used to working on single node modules. For instance, currently in asini if I am in react-server-gulp-module-tagger and run npm run lint, I get an sh: eslint: command not found because eslint is at the project root. I think redfin/react-server#859 is another example.
Personally, I would prefer not to merge this until we've found a way to seed each modules' node_modules/.bin with the tools listed in devDependencies, so that each module in the monorepo still works like a single node module in a propoerly-bootstrapped monorepo. I think you should be able to adapt #357 to such a purpose, or use the utilities we already use for linking internal modules bin scripts.
* Add an option sieve for all commands
Allow config:
- At the top level of `lerna.json`
- In `commands.<command>` in `lerna.json`
- On the command-line
Each of those supercedes previous values.
Example:
```json
{
"lerna": "x.x.x",
"version": "1.2.0",
"ignore": "unrelated-package",
"commands": {
"publish": {
"ignore": "test-package"
}
}
}
```
This deprecates:
- `bootstrapConfig.ignore`
- `publishConfig.ignore`
These still work, with the _lowest_ precedence. Their use triggers a warning.
* Use objectAssign for node 0.10 & 0.12 compatibility
* Factor out lernaJson default empty object
* Slicker legacy option handling courtesy of @rygine
* Just pass options to `buildPackageGraph`
Cleaner interface.
No need to send the whole command object through just for one method.
* Add tests for legacy options
* Add some explanation of the config sieve to the README
Groundwork for pluggable commands. Ensure that command classes: - Are named `*Command` - Are uniquely named - Extend the `Command` base class
This is taken from the base commit on the "enhanced bootstrap" PR.
* Resurrect @rygine's groundwork for common dep hoisting This is taken from the base commit on the "enhanced bootstrap" PR. * Whitespace for legibility Easier on my old eyes while I'm trying to grok this. :) * Update a logger statement This method was renamed while this change set was in flight. * Always warn on un-shared dep (even if already installed) * Only install deps in the root if not already satisfied * Simplify most common version determination * Simplify getDependenciesToInstall Mostly just so I'm sure I understand it. * Pass dependents back from getDependenciesToInstall We'll need those to link binaries from the root. * Actually hoist * Update commentary * Link binaries for hosted dependencies * Don't override an explicit root dependency * Gate hoisting with --hoist and --nohoist options * Update a test to expect slight change to install order * Add a test and fix a bug it uncovered :D Scoped packages weren't hoisted with `--hoist`. * Add a test for --nohoist * Prune hoisted deps from package dirs * Factor out dependent package fetch * Add some documentation about hoisting * Root version may have no dependents If it's depended on by the root package.json and others don't match. * Only warn about mismatch with root if ther's a root version * Start root installs first, since they're likely to take longest * Make tests expect root install first * Don't include null deps in root install These will be present when deps are already satisfied at the root. * Kill a vestigial comment
Ugh. Botched squash and merge. Surprised it worked at all.
Who would put a package at the repo root. That's madness. This eliminates a race condition that causes sporadic failure to bootstrap _this repository specifically_ with hoisting. Will follow up with a PR to actually enable said hoisting.
That's the default. Nothing is hoisted without |
The executable is linked into each dependent package. That is one of the key benefits provided by automatic vs manual hoisting of devDependencies. This broke recently in Asini (and horked React Server). We haven't gotten to fixing it yet because we're hoping that we'll be able to migrate back to Lerna and halt development on Asini. That regression is not part of this PR. |
|
@gigabo OOoooooOOOohhh. Sorry, I misunderstood what was afoot. Lgtm. |
| getOptions(...objects) { | ||
|
|
||
| // Items lower down override items higher up. | ||
| return objectAssign( |
There was a problem hiding this comment.
I think an Object.assign here will get you past the appveyor failure
There was a problem hiding this comment.
Ooh, nice can we use real Object.assign now! 🤘
|
@doug-wade When should we expect an npm release containing this incredible addition? :) |
|
iiuc @hzoo is working on one now. If that is the case, I doubt it'd be more than a couple of days. |
finally, lerna/lerna#507 fixed the devDep issue.
|
@gigabo I'm currently at |
|
Yeah, looking into it now. The port from Asini seems not to have gone well. :( |
|
@noherczeg Hmm... actually I tracked down the hoist slowness to a poor interaction with #547. Filed #568 as a patch for that. That wouldn't affect where things are installed, though. It just makes bootstrap slower than it needs to be. When you run |
|
@gigabo I'm not seeing a single warning during bootstrap. There might be a misunderstanding btw, what I practically meant is that "hoisting does not work at all". Have I missed some prerequisite, or it should just work if I run |
|
I wonder if npm is swallowing your option. You could try terminating npm's options, like: npm run lerna bootstrap -- --hoistOr you could try putting |
|
@gigabo Yes, it was my bad, sorry :( It does work, thank you! |
|
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. |
The main thing preventing React Server from migrating back to Lerna is the lack of automatic hoisting. When I started trying to port that back from Asini, though, I realized that it was built on top of some other improvements... so here's a stack of changes that culminates with an option to automatically hoist common dependencies to the repo root.
The original PRs from Asini for these are:
There was previously some discussion of hoisting in Lerna here.
When I originally did this for Asini I did some perf testing against Babel. I also had a branch with experimental Yarn support, so I compared them. Hoisting actually provided a bigger perf improvement than Yarn. But the best part is they can be combined. Here are bootstrap times on my laptop for Babel with and without hoisting/Yarn: