Skip to content
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

Update to npm 8, pin in package.json via volta and corepack #49941

Merged
merged 5 commits into from
Jul 27, 2022

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Jul 18, 2022

Fixes #49726

But, I'm a bit apprehensive about this just because we have to make sure people have upgraded versions of things.

We'd be better if people just used newer versions of node (16+ ship with npm 8), however, I know many people stick with Node 14 as that's the last version with frame restarting for debugging, and that's why our volta config says what it does.

I have a TODO for the workflow, but I need to wait a few days to be able to see if the command it runs actually changes anything. Otherwise, it's not the right way to update the lockfile. Honestly, it's probably better to just have it delete the lockfile and recreate it from scratch. It's run nightly, so I do not expect conflicts given the frequency of the fixups.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jul 18, 2022
@jakebailey jakebailey added the Infrastructure Issue relates to TypeScript team infrastructure label Jul 18, 2022
@jakebailey jakebailey marked this pull request as draft July 18, 2022 21:19
@jakebailey
Copy link
Member Author

jakebailey commented Jul 25, 2022

There doesn't seem to be a way to emulate npm 6's odd side-effect behavior to use for updating dependencies automatically. IMO that's good (I hate side effects), but for our action we need something, so I've done what I normally do for "update everything" scripts and just have it remove the lockfile. This should be okay because it runs every day.

The only remaining thing to do here is likely to add some sort of documentation that suggests using volta or running corepack enable npm (which should work without any extra installing thanks to corepack's inclusion in current Node 14/16 versions). I also will try and add engines that specify the npm version to. Can't use engines, as that is for consumers of the TS package (e.g. to say "we need at least npm vX for our package to install).

@jakebailey jakebailey marked this pull request as ready for review July 25, 2022 18:55
@jakebailey
Copy link
Member Author

I'm thinking we don't really have to provide any guidance here; I think if people push bad lockfiles to their branches we'll notice and can advise then.

@jakebailey jakebailey changed the title Update to NPM 8, pin in package.json Update to NPM 8, pin in package.json via volta and corepack Jul 25, 2022
@jakebailey jakebailey changed the title Update to NPM 8, pin in package.json via volta and corepack Update to npm 8, pin in package.json via volta and corepack Jul 25, 2022
@jakebailey
Copy link
Member Author

I have absolutely no idea who's the best to review this.

@DanielRosenwasser
Copy link
Member

Definitely the best endorsement I could ever receive.

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

My preferred method of review for this kind of thing is to merge it and see if it causes problems. Seems like you’ve done your research and will recognize if there's some reason we need to roll back.

@jakebailey
Copy link
Member Author

Thanks for the approval.

Do we want to wait for this until after we cut 4.8, or is this low risk enough that it's not going to matter?

@andrewbranch
Copy link
Member

I don’t think this has any impact on release considerations.

@jakebailey jakebailey merged commit 1361567 into microsoft:main Jul 27, 2022
@jakebailey jakebailey deleted the update-lockfile branch July 27, 2022 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug Infrastructure Issue relates to TypeScript team infrastructure
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Upgrade TypeScript lockfile to v2 (upgrade to NPM v7 or higher)
4 participants