Bug: Node 8 support is broken since version v1.3.0

[erezrokah]

In #27 fs-extra was updated to v9 which no longer supports node 8.
See error due to missing optional catch binding in node 8 (https://node.green/#ES2019-misc-optional-catch-binding):

      } catch {
              ^

SyntaxError: Unexpected token {
    at createScript (vm.js:80:10)
    at Object.runInThisContext (vm.js:139:10)
    at Module._compile (module.js:617:28)
    at Object.Module._extensions..js (module.js:664:10)
    at Module.load (module.js:566:32)
    at tryModuleLoad (module.js:506:12)
    at Function.Module._load (module.js:498:3)
    at Module.require (module.js:597:17)

Coming from https://github.com/jprichardson/node-fs-extra/blob/6bffcd81881ae474d3d1765be7dd389b5edfd0e0/lib/mkdirs/make-dir.js#L85

[erezrokah]

This seems to be impacting a bunch of packages (see oclif/config#131 too)

[chadian]

Hi @erezrokah! Really sorry that this has caused issues. oclif aims to track Node's LTS schedule with additional details in this blog post on migrating Node on CLIs. With the current Node LTS schedule, Node 8 is no longer supported. Does the migration blog post help move the netlify CLI forward? A round of major version bumps on oclif could have prevented this issue but I think the hope was to provide the migration path with continued support with Node 10 (or a later LTS).

cc @RasPhilCo

[erezrokah]

Hi @chadian, thank you for the quick reply on this.
We have some internal processes we need to finalise before dropping Node 8 and thank you for sharing the migration guide - that will help a lot.

What led me to file this issue as a bug was:

1. This change was not done behind a major version upgrade - this caused issues for us as we have a process to keep dependencies up to date. That process follows semver versioning.
2. The package.json for this package still supports Node 8:
   

   errors/package.json

   Line 35 in 2233a10

   "node": ">=8.0.0"

We "fixed" this on our end by pinning related packages version to latest known versions that are guaranteed to work on Node 8, but that it not something we want to keep for a long time.

I hope that clarifies the issues we were having.

Dropping Node 8 is definitely the way to go forward and we're pushing for that internally too.

[chadian]

Oh! That is definitely an issue, we need to change that. We do have some tooling that attempts to make assertions around consistent state of packages (including engines.node) but seemed to have missed this. There was a sweep of changes to CI to test supported Node LTS versions which is one reason also why this wasn't caught.

There needs to be a clearer declaration of how versioning is handled. Under semver, you're right, dropping Node is a breaking change and a major version bump. I will try and get some answers/resolutions. If not using semver at least changing the engines.node would have helped prevent this.

[ovr]

Under semver, you're right, dropping Node is a breaking change and a major version bump.

If you don't follow semver, I highly recommend adding a banner in README of all OCLIF's repositories. I am 100% sure that nobody will be happy to get a breaking change problem with any package while he is thinking that that package is using semver.

In fact, it's not possible to hack this problem by resolutions functionality from yarn when you are publishing your package without npm-shrinkwrap.json. fs-extra is a transitive dependency from @oclif/errors that comes from @oclif/command.

I prepared a small PR to get Node.js 8 support back, #101

[chadian]

💯 agreed regarding semver and it being a surprise. There should be better notice if oclif isn't going to follow semver strictly.

Thanks @ovr for your pr, I've merged it and released 1.3.4. Let me know if this fixes the issue.

@ovr, @erezrokah what do you think about an .npmrc file with engine-strict=true and bumping the engines.node property in the package.json? As an interim solution I think this would make clear that the path forward requires a later node version.

[erezrokah]

Thanks @ovr for your pr, I've merged it and released 1.3.4. Let me know if this fixes the issue.

This is great! Before we can verify it (for Netlify CLI) this fix should be ported to https://github.com/oclif/cli-ux/blob/515570bbeb40bcd28698b6b7af53cc9bfdf86279/package.json#L19 and https://github.com/oclif/plugin-plugins/blob/fa800ed279ade4be04d1479297e8c6c8ccd57eed/package.json#L14

@ovr, @erezrokah what do you think about an .npmrc file with engine-strict=true and bumping the engines.node property in the package.json? As an interim solution I think this would make clear that the path forward requires a later node version.

Any solution that makes the installation fail if the Node.js version doesn't match works for me.

[ovr]

Before we can verify it (for Netlify CLI) this fix should be ported to https://github.com/oclif/cli-ux/blob/515570bbeb40bcd28698b6b7af53cc9bfdf86279/package.json#L19 and https://github.com/oclif/plugin-plugins/blob/fa800ed279ade4be04d1479297e8c6c8ccd57eed/package.json#L14

I've prepared two PRs because it's required too from my usages.

[chadian]

@ovr, @erezrokah can you tell me if those fix things or if I have to bump versions for these releases in other oclif packages. I'm going to work on applying the npmrc and engine updates.

[ovr]

@chadian

I am still getting fs-extra ^9 from @oclif/dev-cli

yarn list --pattern fs-extra
├─ @oclif/[email protected]
│  └─ [email protected]

[chadian]

@ovr Thanks, dev-cli has fs-extras downgraded in version 1.23.1

[erezrokah]

Thanks @chadian closing this as resolved. I appreciate the quick response!

[chadian]

Glad it's working out. I've relayed our discussion here about documenting our intention to follow semantic versioning and to include a major version bump for our packages soon with an updated node engine. There are other things being figured out to go along with the major version bumps, too.

Thanks again for everyone's patience, the PRs and feedback.