-
Notifications
You must be signed in to change notification settings - Fork 789
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
fix: ensure api is not dropped from workspaces in package-lock.json #4623
fix: ensure api is not dropped from workspaces in package-lock.json #4623
Conversation
@@ -47,7 +47,7 @@ | |||
"comment_internal": "echo scripts below this line are for internal use", | |||
"_check:no_changes": "if [ ! -z \"$(git status -uall --porcelain)\" ]; then echo Please ensure all changes are committed; exit 1; fi", | |||
"_backup:package-json": "cp package.json package.json.backup", | |||
"_restore:package-json": "mv package.json.backup package.json", | |||
"_restore:package-json": "mv package.json.backup package.json && npm install --package-lock-only", |
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 it would be less processing and safer with something like:
"_backup:package-json": "cp package.json package.json.backup && cp package-lock.json package-lock.json.backup",
"_restore:package-json": "mv package.json.backup package.json && mv package-lock.json.backup package-lock.json",
"safer" because with range dependencies, it is possible that npm install --package-lock-only
can get a different result if there is a newer release of a dep. I'm not actually positive of that. It might depend on the state of the local npm cache. I don't know if/when npm install --package-lock-only
hits the network to get new release information.
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.
Hmm, interesting.
package-lock.json
is updated when I run the lerna update
script, so backing it up before and restoring after, resets it to an inconsistent state. I think we may have to do a npm install --package-lock-only
regardless.
Maybe we can somehow better scope the lerna scripts. I'll try that.
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 looked into npm install --package-lock-only
and it seems that it only changes/updates things if something (package/dependency) is missing. This should never happen on main as npm ci
and all workflows should be failing in such a case. Since we link everything locally, everything should already be updated on release.
I also looked into scoping lerna, and unfortunately we can't easily do this for directories as lerna --scope
operates on the package name 😞
Let's try it with npm install --package-lock-only
first. If we see that it's too fragile I'll look into improving it 🙂
Which problem is this PR solving?
See discussion in #4621, when releasing the API is temporarily removed. This bleeds over into
package-lock.json
where it's never rectified by the_restore:package-json
script.This PR adds a
npm install --package-lock-only
to sync thepackage-lock.json
again after incrementing versions.Type of change
How Has This Been Tested?
prepare_release:sdk:minor