feat(node): drop node 12 support #3302

rwaskiewicz · 2022-03-25T12:33:25Z

Pull request checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)
Build (npm run build) was run locally and any changes were pushed
Unit tests (npm test) were run locally and passed
E2E Tests (npm run test.karma.prod) were run locally and passed
Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

What is the current behavior?

Node 12 will hit End of Life on 2022.04.30. In preparation for that date,
we are adding a PR to the v3 branch. This is a breaking change
that we will not be propagating back to Stencil V2.X.

This commit doesn't intend to set any expectations around v3's release
date. I have some bandwidth on hack days, and wanted to get a jump on
this work. The intent is to merge it into the v3 branch and evaluate other
changes to Stencil (either from main or directly onto the v3 branch)
against this series of changes.

GitHub Issue Number: N/A

What is the new behavior?

This PR is split into several commits, each of which updates a specific part of the project to support Node 14+. For the most part, this has more emphasis on disallowing Node 12 than it does for updating code to run Node 14+. Node's list of deprecated APIs was consulted for version 14+, and any code changes/future TODOs have been added as a part of the PR.

It's probably best that this PR be reviewed commit by commit, so that way the intent of each change is clear (when compared with the git commit msg).

Does this introduce a breaking change?

Yes
No

Testing

CI has been run. We've consulted with the Framework team on this in the past. When we start offering alpha/beta releases, we'll be sure to run them against the Framework team's repo in some way/shape/form

…espectively BREAKING CHANGE

this commit adds two todos to the codebase to add the 'force' flag to `rmdir*` calls in the future, when our typings are updated to a version of Node that supports the flag

alicewriteswrongs

this all looks good to me! just had a few secondary questions

alicewriteswrongs · 2022-04-20T15:53:45Z

.github/workflows/main.yml

@@ -13,7 +13,7 @@ jobs:
  build_and_test:
    strategy:
      matrix:
-        node: ['12', '14', '16']
+        node: ['14', '16']


faster CI! 🚤

alicewriteswrongs · 2022-04-20T16:15:01Z

src/sys/node/node-sys.ts

@@ -344,6 +344,9 @@ export function createNodeSys(c: { process?: any } = {}) {
      return new Promise((resolve) => {
        const recursive = !!(opts && opts.recursive);
        if (recursive) {
+          // TODO(STENCIL-410): In a future version of Node, `force: true` will be required in the options argument. At
+          // the time of this writing, Stencil's Node typings do not support this option.
+          // https://nodejs.org/docs/latest-v16.x/api/deprecations.html#dep0147-fsrmdirpath--recursive-true-


does this mean that if running on node >16 a call to sys.removeDir will throw an error relating to lacking the force: true param? If so, I think the impact may be minimal / nonexistent — a quick grep didn't turn up any places where we actually call the function, although I easily could have missed one

I'm not sure, as the documentation for Node isn't abundantly clear:

In future versions of Node.js, recursive option will be ignored for fs.rmdir, fs.rmdirSync, and fs.promises.rmdir.

Use fs.rm(path, { recursive: true, force: true }), fs.rmSync(path, { recursive: true, force: true }) or fs.promises.rm(path, { recursive: true, force: true }) instead.

It only states that the runtime deprecation began to be fully enforced (whatever that means) in Node v16. I think we'll just need to keep an eye on this as newer versions of Node are release (and we deprecate older ones)

alicewriteswrongs · 2022-04-20T16:16:17Z

package.json

@@ -130,7 +130,7 @@
    "ws": "7.4.6"
  },
  "engines": {
-    "node": ">=12.10.0",
+    "node": ">=14.10.0",
    "npm": ">=6.0.0"


kinda funny to have both the volta config and engines - out of curiosity, do you know why we're specifying engines? the only context where I've had to set it before was to let Heroku know what node version a project should use

engines is for the users of Stencil, where the volta config is for folks developing/hacking on the Stencil compiler itself.

engines doc handles everything but the version of npm/node we should be using for running npm install, running npm scripts, etc in the repo:

You can specify the version of node that your stuff works on (example omitted)
And, like with dependencies, if you don't specify the version (or if you specify "*" as the version), then any version of node will do.
You can also use the "engines" field to specify which versions of npm are capable of properly installing your program. For example: [omitted]

It also isn't strictly enforced:

Unless the user has set the engine-strict config flag, this field is advisory only and will only produce warnings when your package is installed as a dependency.

Volta on the other hand, is strictly enforced when working on Stencil, although it tries to be as transparent/out of the way as possible.

ok that all makes sense to me, thanks for explaining!

alicewriteswrongs · 2022-04-20T16:17:26Z

also I tested out installing this commit into a test component repo (using npm pack) and, as expected, I got an error message w/ node 12 and a looming deprecation warning for node 14.5

rwaskiewicz · 2022-04-20T20:44:29Z

I'm going to merge this - the checks for Node 12 aren't going to pass, because they aren't gonna run (that's the point of this whole PR 🙃). Everything else is passing though 🤞

in #3332, new reusable workflows were created for stencil's ci system to enable parallelism/speed. these workflows were written with stencil v2 in mind, and as a result target node v12. we removed v12 support for stencil (v3+) in #3302. however, upon merging `main` into `v3.0.0-dev`, we realized that we need to remove the values for node 12 from the new workflows

this commit drops node 12 support from stencil by: - removing node 12 from main CI workflow - updating minimum, recommended versions for running stencil to v14.5 and 16.14, respectively - updating volta versions of node, npm for our karma tests - updating the 'engines' field of package.json - setting the minimum default version of node in compiler - adding todos to the codebase for future (Node 18+) work that will be caused by future deprecations STENCIL-176: Drop Node 12 Support BREAKING CHANGE: Node v14+ is now required by Stencil

in #3332, new reusable workflows were created for stencil's ci system to enable parallelism/speed. these workflows were written with stencil v2 in mind, and as a result target node v12. we removed v12 support for stencil (v3+) in #3302. however, upon merging `main` into `v3.0.0-dev`, we realized that we need to remove the values for node 12 from the new workflows

this commit drops node 12 support from stencil by: - removing node 12 from main CI workflow - updating minimum, recommended versions for running stencil to v14.5 and 16.14, respectively - updating volta versions of node, npm for our karma tests - updating the 'engines' field of package.json - setting the minimum default version of node in compiler - adding todos to the codebase for future (Node 18+) work that will be caused by future deprecations STENCIL-176: Drop Node 12 Support BREAKING CHANGE: Node v14+ is now required by Stencil

rwaskiewicz force-pushed the jedi/node14-min branch from 3be104f to a1a92f4 Compare March 25, 2022 12:50

rwaskiewicz changed the title ~~Jedi/node14 min~~ wip(node): drop node 12 support Mar 25, 2022

rwaskiewicz force-pushed the jedi/node14-min branch from 0a195e0 to a733e0d Compare March 28, 2022 14:30

rwaskiewicz added 7 commits April 14, 2022 15:41

chore(ci): remove node 12 from main workflow

eddcee2

feat(bin): update minimum, recommended versions to v14.5 and 16.14, r…

299586f

…espectively BREAKING CHANGE

chore(karma): update volta versions of node, npm

c0a9b34

chore(ci): update build artifact min version

8f57da8

feat(engine): update engines field of package.json

6f5c5a5

chore(compiler): set minimum default version of node in compiler

4975c8d

chore(node): add todos for future rmdir work

f5fc53d

this commit adds two todos to the codebase to add the 'force' flag to `rmdir*` calls in the future, when our typings are updated to a version of Node that supports the flag

rwaskiewicz force-pushed the jedi/node14-min branch 2 times, most recently from 7d6dbca to f5fc53d Compare April 14, 2022 20:03

rwaskiewicz changed the title ~~wip(node): drop node 12 support~~ feat(node): drop node 12 support Apr 14, 2022

rwaskiewicz marked this pull request as ready for review April 14, 2022 20:06

rwaskiewicz requested a review from a team April 14, 2022 20:06

alicewriteswrongs self-assigned this Apr 20, 2022

alicewriteswrongs approved these changes Apr 20, 2022

View reviewed changes

Merge branch 'v3.0.0-dev' into jedi/node14-min

c1bcefe

rwaskiewicz merged commit 72779d9 into v3.0.0-dev Apr 20, 2022

rwaskiewicz deleted the jedi/node14-min branch April 20, 2022 20:49

rwaskiewicz mentioned this pull request Apr 25, 2022

chore(node): remove node 12 from new workflows #3344

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(node): drop node 12 support #3302

feat(node): drop node 12 support #3302

rwaskiewicz commented Mar 25, 2022 •

edited

Loading

alicewriteswrongs left a comment

alicewriteswrongs Apr 20, 2022

alicewriteswrongs Apr 20, 2022

rwaskiewicz Apr 20, 2022

alicewriteswrongs Apr 20, 2022

rwaskiewicz Apr 20, 2022

alicewriteswrongs Apr 20, 2022

alicewriteswrongs commented Apr 20, 2022

rwaskiewicz commented Apr 20, 2022

feat(node): drop node 12 support #3302

feat(node): drop node 12 support #3302

Conversation

rwaskiewicz commented Mar 25, 2022 • edited Loading

Pull request checklist

Pull request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Testing

alicewriteswrongs left a comment

Choose a reason for hiding this comment

alicewriteswrongs Apr 20, 2022

Choose a reason for hiding this comment

alicewriteswrongs Apr 20, 2022

Choose a reason for hiding this comment

rwaskiewicz Apr 20, 2022

Choose a reason for hiding this comment

alicewriteswrongs Apr 20, 2022

Choose a reason for hiding this comment

rwaskiewicz Apr 20, 2022

Choose a reason for hiding this comment

alicewriteswrongs Apr 20, 2022

Choose a reason for hiding this comment

alicewriteswrongs commented Apr 20, 2022

rwaskiewicz commented Apr 20, 2022

rwaskiewicz commented Mar 25, 2022 •

edited

Loading