Skip to content

Conversation

gribnoysup
Copy link
Collaborator

npm >= 7.20 started to run install lifecycle scripts automatically in workspace
subpackages and it is doing it without taking the topology of the packages in
account. This causes install/ci to fail because subpackages are trying to bootstrap
themselves out of order and fail because their workspace dependencies are not ready
yet. This patch switches all prepare scripts to prepublishOnly so that npm does not
run them on install, but they are still activated when running publish with lerna.

…blishOnly

npm >= 7.20 started to run install lifecycle scripts automatically in workspace
subpackages and it is doing it without taking the topology of the packages in
account. This causes install/ci to fail because subpackages are trying to bootstrap
themselves out of order and fail because their workspace dependencies are not ready
yet. This patch switches all prepare scripts to prepublishOnly so that npm does not
run them on install, but they are still activated when running publish with lerna.
@gribnoysup gribnoysup force-pushed the fix-npm-7-topology-issue branch from 495e66b to 51b7016 Compare July 16, 2021 15:37
package.json Outdated
"scripts": {
"bootstrap": "npm install && lerna run prepare --stream",
"bootstrap-evergreen": "npm ci && lerna run prepare",
"bootstrap": "npm install && lerna run prepublishOnly --stream",
Copy link
Collaborator

@mcasimir mcasimir Jul 16, 2021

Choose a reason for hiding this comment

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

Would it be possible to be less implicit here and really just add a bootstrap task to all the packages with that being what we would expect to be called as part of the bootstrap?

I feel it would be better to avoid overloading prepublishOnly, like that we would probably avoid having unrelated tasks being run incidentally in future.

For example we could have probably avoided re-downloading the fonts in the plugins on prepublishOnly since those are only used in development (probably) .. but well I hope it makes sense anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, let's split them up. prepare that was used there before was also running on install and before publish, so this PR just doesn't change the behavior, but your suggestion makes a lot of sense

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 7c071d7, wanna give the PR another look?

@gribnoysup
Copy link
Collaborator Author

Tests are green and bootstrap and prepublish are separated, merging

@gribnoysup gribnoysup merged commit 0922fda into main Jul 21, 2021
@gribnoysup gribnoysup deleted the fix-npm-7-topology-issue branch July 21, 2021 15:31
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.

2 participants