pulsar-edit / ppm

Pulsar Package Manager
MIT License
35 stars 13 forks source link

deps: Upgrade npm to 6.14.18 #53

Closed DeeDeeG closed 1 year ago

DeeDeeG commented 1 year ago

This seems like a good thing to do, to get the latest npm 6.x patch release.

(Changelog, for the curious: https://github.com/npm/cli/blob/v6.14.18/CHANGELOG.md. Nothing big.)

Also syncs the lockfiles with package.json (click to expand for more info): _(This PR properly syncs up `package.json` with both lockfiles `yarn.lock` and `package-lock.json`. I kinda wanted `yarn.lock` to be up-to-date, since `package.json`and `yarn.lock` had gotten ever so slightly out-of-sync, and so it always shows as having uncommitted changes in the `ppm` submodule after bootstrapping the `pulsar-edit/pulsar` repo at the moment... Running `yarn install` and committing the resulting updated `yarn.lock` lockfile in **this repo** means... clean submodule status when bootstrapping `pulsar-edit/pulsar`! Woo, yay!)_
Spiker985 commented 1 year ago

@DeeDeeG Do you know if we'd be able to bump node-gyp as a dev dependency as well? ^5.1.1 is really old, and I think bumping to latest may fix some problems (primarily with detecting new toolchains like Visual Studio 2022).

I can't imagine that having a newer version of node-gyp would prevent the proper install of packages

DeeDeeG commented 1 year ago

@Spiker985, Thanks for the question. Here's the thing... npm doesn't use and won't use that devDependency copy of node-gyp in any meaningful way. The devDependency of node-gyp was added as a top-level devDependency in package.json strictly to make it install to a stable location under node_modules during tests. It moving around broke a certain test that hard-coded its relative path for copying. (Yes, there's a test that copies the node-gyp module folder, it's a long story.)

I proposed to fix the path it installed to and/or update the spec, Amin decided to pin it in package.json instead.

The node-gyp devDependency causes a lot of false hope that we can just bump node-gyp there. I'm here to say that won't help. If it matches npm's desired version, it'll dedupe to no effect. If it doesn't match (newer semver major node-gyp version), npm will install its own copy as its dependency and won't use this one.

To get newer node-gyp, we need newer npm. See: https://github.com/atom-community/apm/pull/123 for npm 8+, https://github.com/DeeDeeG/apm/compare/master...npm-7-compatibility for npm 7.

Spiker985 commented 1 year ago

So apparently, if you use node-pty-prebuilt-multiarch as x-terminal-reloaded does, it tries to run prebuild-install and if it fails, falls back to using node-gyp and if it's being installed through ppm it falls back to ppm's node-gyp

Which is 5.1.0. you can see this in my windows CI from the other day. Line 45. https://github.com/Spiker985/x-terminal-reloaded/actions/runs/4159886533/jobs/7196355988

So, it may unfortunately be in our best interest to bump it anyway alongside the actual node version

Spiker985 commented 1 year ago

I misread my error message, and I believe my error fully stems from npm being out of date

@DeeDeeG You also ~my~ have my approval - do you want to merge?

DeeDeeG commented 1 year ago

Just to reiterate and clarify: To get newer node-gyp that is actually used by ppm in production, we need to bump to a newer major version of npm.

To get newer node-gyp we need npm 7, 8, or 9.

P.S. thanks for the reviews + merge!