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

Use Node 20 #6604

Merged
merged 1 commit into from
Jul 28, 2024
Merged

Use Node 20 #6604

merged 1 commit into from
Jul 28, 2024

Conversation

nyeogmi
Copy link
Contributor

@nyeogmi nyeogmi commented Jul 21, 2024

About The Pull Request

This pull request updates the vendored Node version from 14 to 20.

Why It's Good For The Game

The primary reason for staying on Node 14 was Windows 7 support, a feature only MSO cares about. He isn't a CitadelStation developer and doing a quick poll of the CitadelStation Discord led me to the impression that the other CitadelStation developers also don't care about this.

Switching to Node 20 unbreaks a bizarre behavior I have started seeing when I run the tgui-eslint target. Here's how it looks on Node 14:

>BUILD.cmd tgui-eslint
Using vendored node v14.16.1
:: Juke Build version 0.8.1
:: Skipping 'dm' (up to date)
:: Skipping 'yarn' (up to date)
=> Starting 'tgui-eslint'
:: Skipping 'tgui' (up to date)

Oops! Something went wrong! :(

ESLint: 8.54.0

C:\Nyeogit\Citadel-Station-13-RP\tgui\.yarn\cache\@typescript-eslint-scope-manager-npm-6.12.0-7e7e615f88-4cc4eb1bcd.zip\node_modules\@typescript-eslint\scope-manager\dist\referencer\ClassVisitor.js:123
        withMethodDecorators ||=
                             ^^^

SyntaxError: Unexpected token '||='
    at wrapSafe (internal/modules/cjs/loader.js:979:16)
    at Module._compile (internal/modules/cjs/loader.js:1027:27)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
    at Object.require$$0.Module._extensions..js (C:\Nyeogit\Citadel-Station-13-RP\tgui\.pnp.cjs:24051:33)
    at Module.load (internal/modules/cjs/loader.js:928:32)
    at Function.require$$0.Module._load (C:\Nyeogit\Citadel-Station-13-RP\tgui\.pnp.cjs:23888:14)
    at Module.require (internal/modules/cjs/loader.js:952:19)
    at require (internal/modules/cjs/helpers.js:88:18)
    at Object.<anonymous> (C:\Nyeogit\Citadel-Station-13-RP\tgui\.yarn\cache\@typescript-eslint-scope-manager-npm-6.12.0-7e7e615f88-4cc4eb1bcd.zip\node_modules\@typescript-eslint\scope-manager\dist\referencer\Referencer.js:20:24)
    at Module._compile (internal/modules/cjs/loader.js:1063:30)

Given that we use lockfiles, it's surprising that it's possible for this to be broken on my computer and working on GitHub Actions. That is itself a problem that someone should dig into -- I haven't dug into Juke enough to figure out why this happens.

However, setting this particular case aside -- this feature has existed in JavaScript since 2020 and was supported in Node 15: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Logical_OR_assignment . Library authors are increasingly going to use stuff like this, so if we stay on Node 14, our build is going to break for increasingly esoteric reasons.

Will This Break Anything?

Other than Windows 7? No. Node is primarily used to generate the TGUI bundle, so I tested the behavior of the code we have by generating two bundles, then checking the shasums. On Node 14 I got

tgui.bundle.css: f35775208ffbf8cfa60f6536401f4ae3e80ebe77
tgui.bundle.js: 2a0a01e8163e7e07e3a5db9cd0929c67e67ae1e3
tgui-panel.bundle.css: e3d2dc64c2a550a2dfb83ed7d42e0680b6b25c67
tgui-panel.bundle.js: f8c58cf5c4d59019bfc56a766496a4984c3f3866

On Node 20 I got:

tgui.bundle.css: f35775208ffbf8cfa60f6536401f4ae3e80ebe77
tgui.bundle.js: 2a0a01e8163e7e07e3a5db9cd0929c67e67ae1e3
tgui-panel.bundle.css: e3d2dc64c2a550a2dfb83ed7d42e0680b6b25c67
tgui-panel.bundle.js: f8c58cf5c4d59019bfc56a766496a4984c3f3866

So, it generates apparently identical files.

Also, I spent a while clicking on random objects on Triumph and none of them spawned with broken TGUI.

Changelog

🆑 Pyrex
tweak: Update to Node 20 in the build system.
/:cl:

@github-actions github-actions bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 21, 2024
silicons pushed a commit that referenced this pull request Jul 22, 2024
silicons pushed a commit that referenced this pull request Jul 24, 2024
silicons pushed a commit that referenced this pull request Jul 25, 2024
silicons pushed a commit that referenced this pull request Jul 26, 2024
silicons pushed a commit that referenced this pull request Jul 27, 2024
@silicons silicons merged commit 8dba008 into Citadel-Station-13:master Jul 28, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants