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

require Node >=14 for agoric-sdk #1925

Closed
warner opened this issue Oct 26, 2020 · 10 comments
Closed

require Node >=14 for agoric-sdk #1925

warner opened this issue Oct 26, 2020 · 10 comments
Assignees
Labels
tooling repo-wide infrastructure

Comments

@warner
Copy link
Member

warner commented Oct 26, 2020

Hm, it looks like the test failures are because Node 12.x does not expose WeakRef by default, but Node 14.x does. This might prompt us to increase our required Node.js version to 14, rather than instruct users to add whatever command-line option might enable weakrefs in 12 (if there even is such a thing.. I see --harmony-weak-refs, but it's labelled as "in progress").

Originally posted by @warner in #1924 (comment)

@warner warner added the tooling repo-wide infrastructure label Oct 26, 2020
@warner
Copy link
Member Author

warner commented Oct 26, 2020

@michaelfig noted the Node.js Release Calendar which says Node 14 enters its LTS phase tomorrow (27-Oct-2020). He said v12 is not end-of-life until 30-Apr-2022, and it would be nice to not force people to upgrade to v14.

@erights says SES-shim would probably like to retain Node 12 compatibility. He suggested it would be easy to polyfill/stub WeakRef on platforms where it is not available (basically disabling GC), but was also disinclined to spend significant engineering time on support of an old release.

warner added a commit that referenced this issue Oct 26, 2020
Node.js v14 provides `WeakRef` and `FinalizationRegistry` as globals. Node.js
v12 does not (there might be a command-line flag to enable it, but I think
it's marked as experimental).

Rather than require all users upgrade to v14, we elect to disable GC when
running on v12. This change attempts to pull `WeakRef` and
`FinalizationRegistry` from the global, and deliver either the real
constructors or `undefined` to the liveslots code that uses it. We'll write
that liveslots code to tolerate their lack.

refs #1872
refs #1925
@warner
Copy link
Member Author

warner commented Oct 26, 2020

Let's hold off on this for now, I'll look into a GC-disabling shim for Node v12.

@michaelfig
Copy link
Member

At least Node v12.14.1 (which is our current minimum requirement) implements --harmony-weak-refs to enable WeakRef support. That flag is compatible with v14.x. So, I think it would be fairly straightforward to require that flag for all our top-level scripts and just use WeakRef unconditionally in SwingSet.

warner added a commit that referenced this issue Oct 26, 2020
The upcoming GC functionality will require `WeakRef` and
`FinalizationRegistry`. Node.js v14 provides these as globals, but v12 does
not (there might be a command-line flag to enable it, but I think it's marked
as experimental). Rather than require all users upgrade to v14, we elect to
disable GC when running on v12.

This adds a local `weakref.js` module which attempts to pull `WeakRef` and
`FinalizationRegistry` from the global, and exports either the real
constructors or no-op stubs.

refs #1872
refs #1925
@warner
Copy link
Member Author

warner commented May 20, 2021

In the meeting this morning, we decided to drop support for Node 12. @kriskowal floated the idea of dropping Node 14 as well (to make several -r esm / native-esm-support work better), but we agreed we can't do that until Node 16 reaches LTS.

@warner warner self-assigned this May 20, 2021
@warner warner changed the title require Node 14 for agoric-sdk require Node >=14 for agoric-sdk May 20, 2021
warner added a commit that referenced this issue May 20, 2021
warner added a commit that referenced this issue May 20, 2021
closes #1925
closes #837

The agoric-sdk as a whole requires/advises v14.

We're leaving the individual package.jsons alone until a specific updated
requirement is found for each (e.g. SwingSet will soon bump to v14 because it
will require WeakRef). We won't be testing v12 support anywhere, but it's
possible that individual packages will still work with v12 until we believe
otherwise.

Co-authored-by: Mathieu Hofman <[email protected]>
warner added a commit that referenced this issue May 20, 2021
closes #1925
closes #837

The agoric-sdk as a whole requires/advises v14.

We're leaving the individual package.jsons alone until a specific updated
requirement is found for each (e.g. SwingSet will soon bump to v14 because it
will require WeakRef). We won't be testing v12 support anywhere, but it's
possible that individual packages will still work with v12 until we believe
otherwise.

Co-authored-by: Mathieu Hofman <[email protected]>
warner added a commit that referenced this issue May 20, 2021
BREAKING CHANGE: The agoric-sdk as a whole now requires/advises v14.

We're leaving the individual package.jsons alone until a specific updated
requirement is found for each (e.g. SwingSet will soon bump to v14 because it
will require WeakRef). We won't be testing v12 support anywhere, but it's
possible that individual packages will still work with v12 until we believe
otherwise.

closes #1925
closes #837

Co-authored-by: Mathieu Hofman <[email protected]>
warner added a commit that referenced this issue May 21, 2021
BREAKING CHANGE: The agoric-sdk as a whole now requires/advises v14.

We're leaving the individual package.jsons alone until a specific updated
requirement is found for each (e.g. SwingSet will soon bump to v14 because it
will require WeakRef). We won't be testing v12 support anywhere, but it's
possible that individual packages will still work with v12 until we believe
otherwise.

closes #1925
closes #837

Co-authored-by: Mathieu Hofman <[email protected]>
warner added a commit that referenced this issue May 21, 2021
BREAKING CHANGE: The agoric-sdk as a whole now requires/advises v14.

We're leaving the individual package.jsons alone until a specific updated
requirement is found for each (e.g. SwingSet will soon bump to v14 because it
will require WeakRef). We won't be testing v12 support anywhere, but it's
possible that individual packages will still work with v12 until we believe
otherwise.

closes #1925
closes #837

Co-authored-by: Mathieu Hofman <[email protected]>
@warner warner closed this as completed in f014684 May 21, 2021
@turadg
Copy link
Member

turadg commented Jan 25, 2022

In the meeting this morning, we decided to drop support for Node 12. @kriskowal floated the idea of dropping Node 14 as well (to make several -r esm / native-esm-support work better), but we agreed we can't do that until Node 16 reaches LTS.

Now that Node v16 is LTS shall we drop v14?

@michaelfig
Copy link
Member

Now that Node v16 is LTS shall we drop v14?

I would prefer a "last 2 releases" policy. Is node 18 a thing yet?

@turadg
Copy link
Member

turadg commented Jan 25, 2022

No Node 18 yet, though Node 17 is released so <=15 is older than "last 2 releases". https://nodejs.dev/download/ has a table of the releases and timing, showing that v16 is the "Active LTS" and v14 is what's called a "Maintenance LTS".

Why the preference for supporting older than the Active LTS?

@michaelfig
Copy link
Member

Why the preference for supporting older than the Active LTS?

@kriskowal has settled on supporting from v12 on the current endo, so I don't think his request implies we need to drop v14. I only want to drop v14 support if it actually improves the SDK, and I'm not convinced of that yet. Do you have reasons?

My hesitance is due to an aversion for breaking changes.

@turadg
Copy link
Member

turadg commented Jan 25, 2022

Makes sense. The reasons I thought it might be good to drop Node 14,

  • ESM support mentioned above apparently irrelevant to this repo
  • cut CI time (11 jobs listed below totalling 47min), though it wouldn't necessarily make the PR finish faster
  • features of v16 such as,
    • Experimental support for Web Crypto API
    • Timers Promises API
    • npm 7
    • Stable AbortController
    • Stable Source Maps v3
PR checks that would be cut
Test all Packages / build (14.x) (pull_request) Successful in 4m
Test all Packages / test-quick (14.x) (pull_request) Successful in 5m
Test all Packages / test-quick2 (14.x) (pull_request) Successful in 4m
Test all Packages / test-solo (14.x) (pull_request) Successful in 4m
Test all Packages / test-cosmic-swingset (14.x) (pull_request) Successful in 6m
Test all Packages / test-swingset (14.x) (pull_request) Successful in 4m
Test all Packages / test-swingset2 (14.x) (pull_request) Successful in 3m
Test all Packages / test-swingset3 (14.x) (pull_request) Successful in 3m
Test all Packages / test-swingset4 (14.x) (pull_request) Successful in 4m
Test all Packages / test-zoe-unit (14.x) (pull_request) Successful in 5m
Test all Packages / test-zoe-swingset (14.x) (pull_request) Successful in 5m

I don't strongly advocate one way or the other. Mostly I'd like to see the decision rationale documented in the repo, which I'm happy to do.

@kriskowal
Copy link
Member

I definitely intend to withdraw support for Node.js 12, though I’ve decoupled that from recent changes where I managed to keep support. Best reason to drop it is the experimental and different ESM implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling repo-wide infrastructure
Projects
None yet
Development

No branches or pull requests

4 participants