Skip to content
This repository was archived by the owner on Mar 3, 2023. It is now read-only.

bootstrap: Use legacy-compatible catch syntax (support older Node) #22979

Merged
darangi merged 1 commit into from
Sep 20, 2021
Merged

bootstrap: Use legacy-compatible catch syntax (support older Node) #22979

darangi merged 1 commit into from
Sep 20, 2021

Conversation

@DeeDeeG
Copy link
Contributor

@DeeDeeG DeeDeeG commented Sep 14, 2021 •
edited
Loading

Requirements for Contributing a Bug Fix (from template, click to expand)

Identify the Bug

Fixes #22969

Description of the Change

  • Use catch syntax in script/lib/verify-machine-requirements.js that is backward-compatible with older Node (9 and older)
    • Node 10 introduced catch {} blocks without needing to bind the exception. Older Node requires the exception to be bound, like this: catch (exceptionVarName) {}.

Side note:

  • (You can't actually build Atom with any Node version less than 10.12 anymore, but the system requirements checker script really should support older versions of Node... If only so we can print more-useful errors about users' versions of Node being too old.)
  • We want the script to get far enough to print a nice error telling them their Node is too old, not throw an obscure syntax error about try/catch block syntax! (Bootstrap syntax error #22969).

Alternate Designs

None.

Possible Drawbacks

Allows the rather outdated Node check to function as written.

The Node version check should be updated... Please consider merging #23001 along with this PR.

Verification Process

  • Ranscript/bootstrap and script/build on Node 9 and older.

Result: Users with Node older than 10 who run script/bootstrap (or script/build) can now get through the earliest part, script/lib/verify-machine-requirements.js, without encountering syntax errors. That means the script can properly do its job of detecting build dependency versions and printing their version numbers... and/or printing warnings or errors if the versions are too old to build Atom.

Release Notes

N/A

@DeeDeeG
bootstrap: Fix syntax error for Node older than 10
5339e5d 
We don't actually support Node older than 10 for successfully
bootstrapping an Atom build, but we should support older Node in the
system requirements checker script so that we can print useful errors,
like ones to tell the user their system's copy of Node is too old!

(It was me that introduced this Node 10.0.0 or above-only syntax
about a year ago... My mistake. It's as good a time as any to fix it!)
@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented Sep 14, 2021 •
edited
Loading

The syntax error (#22969) (catch {} block without binding e, like this: catch (e) {}) is strictly a bug (... which only affects older unsupported Node versions... but the system requirements checker is meant to tell you whether your Node is too old! Not print obscure syntax errors! This PR fixes that situation.)

Updating the minimum allowed Node to 10.12+ is correct, but arguably not strictly a bug fix... These two changes are so closely thematically related though, I thought it best to put them in one PR. Happy to separate them out if needed.

Edit: Moved tangentially related fix to #23001.

@DeeDeeG DeeDeeG force-pushed the update-system-requirements-checker-node branch from 7ffe869 to 5339e5d Compare September 18, 2021 20:29
@DeeDeeG DeeDeeG changed the title Update Node compatibility in script/lib/verify-machine-requirements.js Use legacy-compatible catch block syntax in script/lib/verify-machine-requirements.js Sep 18, 2021
@DeeDeeG DeeDeeG changed the title Use legacy-compatible catch block syntax in script/lib/verify-machine-requirements.js Use legacy-compatible catch block syntax in script/lib/verify-machine-requirements.js (support older Node) Sep 18, 2021
@DeeDeeG DeeDeeG mentioned this pull request Sep 18, 2021
Merged
@DeeDeeG DeeDeeG changed the title Use legacy-compatible catch block syntax in script/lib/verify-machine-requirements.js (support older Node) Use legacy-Node-compatible catch block syntax in script/lib/verify-machine-requirements.js (support older Node) Sep 18, 2021
@DeeDeeG DeeDeeG changed the title Use legacy-Node-compatible catch block syntax in script/lib/verify-machine-requirements.js (support older Node) Use legacy-compatible catch block syntax in script/lib/verify-machine-requirements.js (support older Node) Sep 18, 2021
@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented Sep 18, 2021 •
edited
Loading

I split this PR in two, since the changes were technically unrelated/served slightly different purposes... If merging this, please also consider #23001. The current PR makes the version checker run on older Node, which is a good thing IMO, but the version the script checks for hasn't been updated in a long time and is quite outdated. See #23001 to raise the version of Node being checked to something more appropriate.

@DeeDeeG DeeDeeG changed the title Use legacy-compatible catch block syntax in script/lib/verify-machine-requirements.js (support older Node) bootstrap: Use legacy-compatible catch syntax (support older Node) Sep 18, 2021
@darangi
Copy link
Contributor

darangi commented Sep 20, 2021

Thanks for the contribution 🙇🏾

@darangi darangi merged commit dd4fae1 into atom:master Sep 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bootstrap syntax error

2 participants

@DeeDeeG @darangi