-
Notifications
You must be signed in to change notification settings - Fork 370
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: update package-lock.json workspace entiry versions #2088
feat: update package-lock.json workspace entiry versions #2088
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
@@ -34,6 +34,16 @@ export class PackageLockJson extends DefaultUpdater { | |||
if (parsed.lockfileVersion === 2 || parsed.lockfileVersion === 3) { | |||
parsed.packages[''].version = this.version.toString(); | |||
} | |||
if (this.versionsMap) { | |||
for (const [p, obj] of Object.entries(parsed.packages)) { |
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.
Reading this not after midnight, I can probably clean this up a bit. But before I do I would like to get a general go ahead from a maintainer.
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.
Feel free to clean this up in a follow up
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.
Ok, yeah I don't think it was too bad, but it took me a second in the morning to read that and remember what exactly I had done. If you are good with it on this PR I honestly am good with just leaving it since it really isn't that bad and does appear to fully resolve the issue.
@chingor13 could I get some eyes on this? We are working toward using RP in a tool at Netflix and without this patch it is really painful to release a monorepo. It requires pulling down the release branch, running install, then ammending and force pushing. I tried to setup an automated action base workflow, but it has drawbacks and I was hoping this could land so I don't need to perfect that workaround. Especially since this was a pre-existing ask and I ensuring the locks are bumped will benefit all users. |
I'm not a maintainer, but I'd also be keen to see this land! |
@chingor13 sorry for the ping again, not sure if there are other folks who I can ping to look at this, but it would be nice if we could land this soon. Thanks! |
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.
Sorry for the delay in getting to this, but thanks!
@@ -34,6 +34,16 @@ export class PackageLockJson extends DefaultUpdater { | |||
if (parsed.lockfileVersion === 2 || parsed.lockfileVersion === 3) { | |||
parsed.packages[''].version = this.version.toString(); | |||
} | |||
if (this.versionsMap) { | |||
for (const [p, obj] of Object.entries(parsed.packages)) { |
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.
Feel free to clean this up in a follow up
2501c20
to
a010d68
Compare
Fixed the lint issues on my branch. |
for (const [name, ver] of this.versionsMap?.entries()) { | ||
if (obj.name === name) { | ||
obj.version = ver.toString(); | ||
} | ||
} |
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.
Reading this again, this could be slightly more performent:
for (const [name, ver] of this.versionsMap?.entries()) { | |
if (obj.name === name) { | |
obj.version = ver.toString(); | |
} | |
} | |
const ver = this.versionsMap.get(name); | |
if (ver) { | |
obj.version = ver.toString(); | |
} |
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.
oh ha, thats funny I think I had written the original code assuming what I had there was a list and then instead of just reading it again when I modified it to take the map I just.....left it lol. Will fix that.
EDIT: oh yeah, it was the middle of the night...makes sense lol
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.
Fixed
a010d68
to
34d4608
Compare
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #1993 🦕