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

Drop Node.js required version from 8.15.0 to 8.7.0 #4032

Merged
merged 4 commits into from Apr 16, 2019

Conversation

3 participants
@ntwb
Copy link
Member

commented Apr 15, 2019

Via #4000 (comment)

As AWS Labda currenty only supports Node.js 8.10, let's drop the required version down to 8.0.0

We can add this to the next minor release 10.0.1

ntwb added some commits Apr 15, 2019

@hudochenkov

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

What if we, or our dependency, uses a feature introduced in 8.3.0 for example?

@jeddy3

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

What if we, or our dependency, uses a feature introduced in 8.3.0 for example?

I tested locally with 8.0.0 and stylelint crashes out with

/Users/jeddy3/Projects/stylelint/node_modules/micromatch/index.js:44
    let isMatch = picomatch(String(patterns[i]), { ...options, onResult }, true);
                                                   ^^^
SyntaxError: Unexpected token ...

This is because micromatch uses the object spread operator which is behind a flag until 8.3.0.

micromatch has >=8 in its package.json, which gives me the impression that this engine field is a bit problematic.

According to node.green the following versions added new features:

  • 8.7.0
  • 8.3.0

Even though stylelint works with 8.3.0 I suggest we go with 8.7.0 as that's the lowest feature-complete version of 8 (and it's compatible with AWS). This seems to me to be the best compromise.

@jeddy3 jeddy3 referenced this pull request Apr 15, 2019

Closed

Release 10.0.1 #4033

4 of 4 tasks complete

ntwb added some commits Apr 15, 2019

@ntwb

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

Thanks, makes sense, have bumped it to 8.7.0

@jeddy3

jeddy3 approved these changes Apr 15, 2019

Copy link
Member

left a comment

LGTM.

@jeddy3 jeddy3 changed the title Drop Node.js required version from 8.15.0 to 8.0.0 Drop Node.js required version from 8.15.0 to 8.7.0 Apr 15, 2019

@jeddy3

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

@hudochenkov Did you have any more thoughts on this? I can put out a patch release this morning if this goes in.

@hudochenkov
Copy link
Member

left a comment

LGTM!

@jeddy3 jeddy3 merged commit 1d204c2 into master Apr 16, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.001%) to 96.365%
Details

@jeddy3 jeddy3 deleted the drop-node-version branch Apr 16, 2019

@jeddy3

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

  • Fixed: minimum Node.js engine reduced to 8.7.0 (#4032).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.