-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add all monorepo packages as top-level deps #39218
Conversation
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
"requires": { | ||
"babel-runtime": "^6.9.2", | ||
"debug": "^3.1.0", | ||
"@babel/runtime": "^7.8.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iiiiinteresting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's caused by the recent wpcom
PRs that upgraded the package's dependencies, but not the lockfile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, surprised npm ci
wasn't yelling about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that npm ci
doesn't look at the requires
fields at all. It just blindly recreates the node_modules
tree from the nested dependencies
objects in the lockfile. For that, only the tree structure and the resolved
and integrity
fields are needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests out fine, looks good.
Add all packages in the monorepo as dependencies of the top-level
package.json
. Some of them already are there asdevDependencies
. Here we are adding the rest.This is a workaround for NPM bug: npm/cli#750
How to test:
Do a clean (no
node_modules
) install (notci
) from the lockfile:Before this patch, the
npm install
failed to create symlinks to packages like@automattic/components
and even removed the dependency from the lockfile.After the patch,
npm install
should install everything just likenpm ci
and shouldn't change the lockfile at all.