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 8 #4500

Merged
merged 3 commits into from Jan 9, 2020
Merged

Drop Node.js 8 #4500

merged 3 commits into from Jan 9, 2020

Conversation

hudochenkov
Copy link
Member

End of life on 31st December.

package.json Outdated
@@ -40,7 +40,7 @@
"!lib/testUtils"
],
"engines": {
"node": ">=8.7.0"
"node": ">=10"
Copy link
Member

Choose a reason for hiding this comment

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

maybe better start to support with LTS, i.e. 10.13.0

Copy link
Member Author

Choose a reason for hiding this comment

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

Some users had problem, when we set minor version too high: #4000. But it's not longer the case:

Starting with Node 10, AWS Lambda will also automatically update the language minor versions to latest minor version, as specified by https://github.com/nodejs/Release.

https://aws.amazon.com/about-aws/whats-new/2019/05/aws_lambda_adds_support_for_node_js_v10/

So I guess we can even put latest 10.18.0 as the minimum.

Copy link
Member

Choose a reason for hiding this comment

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

I think allowing for edge cases like what happened in #4000 is fine, min v10.0.0 I think will be ok

Copy link
Member

Choose a reason for hiding this comment

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

Unless a specific feature is required from a version newer than 10.0.0, we can just go with 10.0.0. Just note that CI tests the latest release line and this can be problematic in some cases. It might be wiser to test the minimum supported Node.js version at the time instead.

package.json Outdated
@@ -40,7 +40,7 @@
"!lib/testUtils"
],
"engines": {
"node": ">=8.7.0"
"node": ">=10"
Copy link
Member

Choose a reason for hiding this comment

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

I think allowing for edge cases like what happened in #4000 is fine, min v10.0.0 I think will be ok

@hudochenkov
Copy link
Member Author

hudochenkov commented Jan 1, 2020

I would declare minimum supported version as latest Node.js 10 LTS release, but remove engine field. It's similar to strategy that many websites use: support last two major browse releases.

We might not be using any Node.js latest features, but I don't want to be locked in old releases. I work on a free software and spent my life to work on it. Users can upgrade their Node.js version with no problem.

If stylelint works on Node.js 10.0.0. It's great, but it won't be supported version. Removing engine field will allow to run on “unsupported” Node.js versions without, e. g. yarn throwing and error about incompatibility.

Also consider our dependencies which specify they drop Node.js 8. We don't know on what exact version of Node.js 10 they will work (except the very latest release). If we specify supported version as 10.0.0, but our dependency needs 10.2.0, we kind of messing with our users.

@hudochenkov hudochenkov requested a review from ntwb January 2, 2020 01:03
@ntwb
Copy link
Member

ntwb commented Jan 2, 2020

Looks like we'd have to update eslint-config-stylelint with this:

https://github.com/mysticatea/eslint-plugin-node/blob/master/docs/rules/no-unsupported-features/es-syntax.md#options

If you omit the engines field, this rule chooses >=8.0.0 as the configured Node.js version since 8 is the minimum version the community is maintaining (see also Node.js Release Working Group).

{
    "node/no-unsupported-features/es-syntax": ["error", {
        "version": ">=10.0.0",
        "ignores": []
    }]
}

@hudochenkov
Copy link
Member Author

As per my comment (#4500 (comment)) I put latest v10 LTS into ESLint config.

@hudochenkov hudochenkov mentioned this pull request Jan 5, 2020
7 tasks
@hudochenkov
Copy link
Member Author

@stylelint/contributors does anyone have time to review this, please?

Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

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

+1

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

Agree!

@hudochenkov hudochenkov merged commit dd33451 into master Jan 9, 2020
@hudochenkov hudochenkov deleted the drop-node-8 branch January 9, 2020 08:58
@hudochenkov
Copy link
Member Author

  • Removed: Node.js 8.x support. Node.js 10 is now required. We can guarantee stylelint works on the latest Node.js 10 release. (#4500).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants