Upgrade Node to version 20.11.1

[savetheclocktower]

Inspired by #153, I decided to tilt at this windmill, and in particular to upgrade the specs to the latest version of Jasmine.

This was a chore, but a straightforward one, and it let me shed a lot of boilerplate. And once I bumped the ancillary fixture artifacts to agree with Node 20.11.1, the last few tests started passing! (For me locally, at least. We'll see how humbling the CI run is.)

[DeeDeeG]

Essential info is bolded, the rest is fluff!

(This is a not-very-obvious thing, I do not expect this to be apparent without a mention. So here I am pointing it out...!)

Please consider using Electron headers rather than Node headers for the spec fixtures. (They are smaller is the main motivation. Also, more-closely tests with a like-for-like header version as we anticipate using in Pulsar core, to the extent that ends up mattering.) The electron header files can be fetched from electron.org servers by poking URLs with the following scheme: e.g. https://artifacts.electronjs.org/headers/dist/v17.0.0/node.lib; https://artifacts.electronjs.org/headers/dist/v30.0.9/node-v30.0.9-headers.tar.gz

(See: https://www.electronjs.org/blog/s3-bucket-change for the lore behind these URLs if curious.)

The filename scheme is inherited from Node stuff for tooling compatibility, so the immediate impression is, "of course these are Node headers and libs, etc.!"

(Story time: I only remembered the Electron headers existed and could be used here after committing some rather large Node 10 headers once... their bloat haunts the repo now when git cloneing...)

(TANGENT: This also makes me realize we should be looking into getting "Electron 30 in core" stuff to be 30.5.1, not 30.0.9.

I found this tool today to check the release history: https://releases.electronjs.org/release?major=v30 and it made me realize 30.0.9 isn't the most-recent Electron 30 anymore!

But that's there not here!)

[savetheclocktower]

You're definitely correct about targeting 30.5.1 instead of 30.0.9, and I wish I had thought of that sooner.

At this point, I'm more inclined to ship what we have and then focus on having the subsequent release of Pulsar be on either 30.5.1 or a newer major Electron version.

Anyway, I'm taking this out of draft. If we're OK with landing it, I'll then bump my Electron 30 PR to use the latest ppm and then we can test it holistically — both ppm and Pulsar on the same version of Node.

EDIT: But, yes, I'll incorporate your feedback above first!

[DeeDeeG]

The last PR where I bumped the spec fixture headers and such may be of interest for historical context: #52

Specifically important: The fixture file named node_x64.lib in this repo comes from the win-x64/node.lib path on electron.org's servers.

(We might be able to use the same directory structure and naming as electron.org's servers, instead of renaming and moving win-x64/node.lib to node_x64.lib, to be less confusing for future updates of these fixtures. I dunno. Haven't thought about it in a while, TBH.)

[DeeDeeG]

It's kind of fiddly to update the spec fixture files, and I've refreshed my memory of how to do it (via a trip down memory lane), so I can get those files updated if you like. But I also respect the "passing down of knowledge" aspect if you prefer to do it yourself and learn how hands-on. (So I will assume you want to do it as indicated above, but feel free to request me to do it if it's preferable to have one more fiddly task off of your plate.)

EDIT: I note you've already gotten it right once already, given the tests aren't exploding! Well done there! I recall it being a bit confusing and reverse-engineering-ish when I first did it.

[DeeDeeG]

Doin' a little rebasing so the commits with Node headers checked in to the git repo are gone from the history, to keep fetches of master branch from needing to get those headers as objects (several MB large). As in, keeps the git repo lighter for cloning.

(Almost properly discussed on Discord beforehand! Mostly!)

Old commits saved to https://github.com/pulsar-edit/ppm/commits/old-pr-156/ for posterity. (Tip of branch a2152d8.)

[DeeDeeG]

I can confirm, on my macOS 10.15.7 computer, that the specs seem to be running equivalently.

Details

On master:

% ./bin/npm test

> [email protected] test /Users/User/ppm
> npm run check-version && node ./script/local-jasmine.js --captureExceptions spec


> [email protected] check-version /Users/mate/ppm
> node script/check-version.js

..........................................................................................................................................................

Finished in 104.162 seconds
154 tests, 450 assertions, 0 failures, 0 skipped

---

On node-20 (this PR branch):

% ./bin/npm test

> [email protected] test /Users/User/ppm
> npm run check-version && jasmine --config=./spec/jasmine.json


> [email protected] check-version /Users/mate/ppm
> node script/check-version.js

Started
..........................................................................................................................................................


154 specs, 0 failures
Finished in 63.469 seconds

Same number of specs, and all passing, according to the readout.

Will see if there is much else to comment on, but looking good so far (still trawling through some of the files/diff.)

[DeeDeeG]

I have some minor nit-picks:

• I'm not sure how I feel about the waterfall/pyramid-of-doom indentation for the app.get() calls for express in various spec files for readability, but this is such a minor point I'm not really gonna be too bothered if it stays.
• Some oddnes and churn in yarn.lock such as registry.npmjs.org instead of registry.yarnpkg.com ... I restored yarn.lock from master and reverted the second-mate change in package.json and re-ran yarn install for a much smaller and more normal-looking diff. I can push those two changed files if you like, but for cleaner history I might prefer the churn be rebased out entirely. But this isn't such a sticking point that I can't hear a counter argument.

But overall looking good, I would save my writeup of what I like for the approve, but it is pretty nice IMO!

[savetheclocktower]

I certainly think the yarn.lock nitpick is worth adopting. I don't know what caused that much churn in the lockfile, but I'm definitely in favor of smaller diffs. Please do push the lockfile changes, and rewrite history however you like!

[DeeDeeG]

Alrighty, once again, old commits are at https://github.com/pulsar-edit/ppm/commits/old-pr-156-2 (tip of branch 63499eb).

Pushed to yeet the large majority of the yarn.lock churn and not need to mess with second-mate in package.json. Hopefully CI is still happy. UPDATE: Yep, it passed. 👍

Thanks.

[DeeDeeG]

Lots less boilerplate in the specs, a more modernized Node version (and matching the anticipated Electron 30 bump in core), a more modernized Jasmine, a script to automate getting newer spec fixtures. All pretty cool, thank you for these things!

The diff to specs is rather small outside of the boilerplate reduction, some promisification and usage of async/await, and whitespace/indentation stuff, which is great, IMO.

Note: I think the old local-jasmine stuff was a workaround for some jasmine-focused funkiness, if I'm reading #87 correctly. So, hopefully moving away from jasmine-focused entirely is for the best. It was always a bit fiddly to have a patched Jasmine, I reckon. Probably for the best to move past that. And for a much newer Jasmine, so, yeah.

As mentioned above, I tested locally and tests passed there as well as them passing in CI.

LGTM, thanks once again, this seems like a bigger change to get done, but pretty smooth to review considering.