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

Breaking: require Node.js 6+, upgrade [email protected] (fixes #345) #371

Merged
merged 10 commits into from
Mar 23, 2018

Conversation

aladdin-add
Copy link
Member

No description provided.

Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

package.json Outdated
@@ -10,7 +10,7 @@
"espree.js"
],
"engines": {
"node": ">=0.10.0"
"node": ">=4.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

Since we're doing a breaking change, should we change this to >=6.0.0 to be the same as ESLint v5?

@@ -47,7 +47,7 @@ module.exports = {
0,
8
],
"value": null,
"value": process.versions.node >= "9.0.0" ? new RegExp(String.raw`(?<!a)`) : null,
Copy link
Member Author

Choose a reason for hiding this comment

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

emm, this failed. seems Node.js 8 doesn't support these features, checking Node.js version seems not a good idea, thougths?

Copy link
Member

Choose a reason for hiding this comment

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

For unicode regexes, the tests use the conditionalRegex helper. Would that work here?

It's not related to the issue, but I think using >= "9.0.0" will fail when Node 10.0.0 comes out, because it compares the version alphabetically.

@@ -1,3 +1,5 @@
const condtionalRegex = require("../../../../lib/conditional-regex-value");
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: I think the variable name is misspelled here (it is missing an "i" after "cond").

@aladdin-add aladdin-add changed the title Breaking: upgrade [email protected] (fixes #345) Breaking: require Node.js 6+ & upgrade [email protected] (fixes #345) Mar 15, 2018
@aladdin-add aladdin-add changed the title Breaking: require Node.js 6+ & upgrade [email protected] (fixes #345) Breaking: require Node.js 6+, upgrade [email protected] (fixes #345) Mar 15, 2018
Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

LGTM

@not-an-aardvark not-an-aardvark merged commit 0df063f into eslint:master Mar 23, 2018
@aladdin-add aladdin-add deleted the issue345 branch April 1, 2018 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants