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.

Sign up for GitHub

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

feat(node): drop node 12 support #3302

Merged
merged 8 commits into from
Apr 20, 2022
Merged

feat(node): drop node 12 support #3302

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
eddcee2
chore(ci): remove node 12 from main workflow
rwaskiewicz Mar 4, 2022
299586f
feat(bin): update minimum, recommended versions to v14.5 and 16.14, r…
rwaskiewicz Mar 4, 2022
c0a9b34
chore(karma): update volta versions of node, npm
rwaskiewicz Mar 4, 2022
8f57da8
chore(ci): update build artifact min version
rwaskiewicz Mar 4, 2022
6f5c5a5
feat(engine): update engines field of package.json
rwaskiewicz Mar 4, 2022
4975c8d
chore(compiler): set minimum default version of node in compiler
rwaskiewicz Mar 24, 2022
f5fc53d
chore(node): add todos for future rmdir work
rwaskiewicz Mar 24, 2022
c1bcefe
Merge branch 'v3.0.0-dev' into jedi/node14-min
rwaskiewicz Apr 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/sys/node/node-sys.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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-
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

fs.rmdir(p, { recursive: true }, (err) => {
resolve({
basename: path.basename(p),
Expand Down Expand Up @@ -372,6 +375,9 @@ export function createNodeSys(c: { process?: any } = {}) {
try {
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-
fs.rmdirSync(p, { recursive: true });
} else {
fs.rmdirSync(p);
Expand Down