-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
feat(package): .serialize() also writes package-lock version #2160
feat(package): .serialize() also writes package-lock version #2160
Conversation
90199a6
to
2ec8639
Compare
0ab7309
to
a49d984
Compare
It occurs to me that while this update will bump the version property in the |
@@ -77,6 +86,9 @@ class Package { | |||
manifestLocation: { | |||
value: path.join(location, "package.json"), | |||
}, | |||
lockFileLocation: { | |||
value: path.join(location, "package-lock.json"), |
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.
this could be npm-shrinkwrap.json
, or yarn.lock
, or, or, or...
writeJsonFile(this.lockFileLocation, updatedLockFileData, { detectIndent: true }), | ||
]; | ||
|
||
return hasPackageLock(this.lockFileLocation) ? pWaterfall(tasks) : Promise.resolve(); |
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 don't think we need p-waterfall
just to chain promises. This check should be done way earlier, too.
@@ -7,7 +7,7 @@ | |||
"ci": "npm test -- --ci --maxWorkers=2 && npm run integration -- --ci", | |||
"fix": "npm run lint -- --fix", | |||
"integration": "jest --config jest.integration.js --maxWorkers=2", | |||
"lint": "eslint . --ignore-path .gitignore --cache --cache-location ./node_modules/.cache/eslint", | |||
"lint": "eslint . --ignore-path .gitignore --ignore-pattern \"**/node_modules/\" --cache --cache-location ./node_modules/.cache/eslint", |
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.
Unclear why this is necessary, as it is already ignored.
@@ -30,6 +33,12 @@ function shallowCopy(json) { | |||
}, {}); | |||
} | |||
|
|||
// determine if a package-lock.json file exists |
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.
This isn't the appropriate place for lockfile maintenance. It's a function of the version command, not integral to the nature of a Package.
Description
The
@core/package
class has a method called.serialize()
which writes the in-memory package to disk. When runninglerna version
the.version
property of the package is updated, written, and committed (depending on flags). However, thepackage-lock.json
file (which also has a top levelversion
property) doesn't get updated, and is then out of sync with thepackage.json
file.This change updates the
serialize()
function to check if the package has apackage-lock.json
file in the directory, and if it does: read it in, update the version, then write it to disk.Motivation and Context
Any monorepo project that has
package-lock.json
files in it's package folders need to run another step to get thepackage-lock.json
files to stay in sync with thepackage.json
files. The script usually looks something like:(or some such variation of the above)
Solves: #1998
Linked: #1415
How Has This Been Tested?
Tests where added to the
core/package/__tests__/core-package.test.js
file.describe("get .lockFileLocation")
was addedit("should return the package-lock location")
describe(".serialize()")
was updatedit("writes changes to package.json to disk")
updatedit("writes changes to package-lock to disk")
addedTypes of changes
Checklist: