Upgrade: eslint-plugin-node to 7.0.1

[mysticatea]

What is the purpose of this pull request? (put an "X" next to item)

[X] Other, please explain: upgrade eslint-plugin-node to 7.0.1 for linting our codebase.

What changes did you make? (Give an overview)

It has new features:

• It reports the APIs of Node.js built-in modules and globals if those are not supported on the configured Node.js versions (^6.14.0 || ^8.10.0 || >=9.10.0).
• It reports newly deprecated APIs.

Is there anything you'd like reviewers to focus on?

I'm not sure if this util.isDeepStrictEqual() part is needed.
That looks to do double checking for the AST if util.isDeepStrictEqual exists. But why?

// Feature detect the Node.js implementation and use that if available.
if ((util.isDeepStrictEqual && !util.isDeepStrictEqual(beforeAST, afterAST)) || !lodash.isEqual(beforeAST, afterAST)) {

[aladdin-add]

I'm not sure if this util.isDeepStrictEqual() part is needed.
That looks to do double checking for the AST if util.isDeepStrictEqual exists. But why?

seems we have been inconsistent in the usage of _. isEqual vs.util.isDeepStrictEqual. it can be restricted in the eslintrc( but in another PR)

[platinumazure]

I'm a bit confused about this line. Is the name of the rule "no-unsupported-features/node-builtins" (and therefore the plugin-qualified name is "node/no-unsupported-features/node-builtins"? I don't think I've ever seen a rule defined with a forward slash in its name, just wanted to make sure I understood correctly.

[mysticatea]

Yes. We can use / in the rule name in plugins. #3458 (comment) mentioned this fact.

[platinumazure]

You might be right about this running two deep equal checks. Maybe it would be worthwhile to extract the logic, or change to a ternary (if (!(util.isDeepStrictEqual ? util.isDeepStrictEqual(beforeAST, afterAST) : lodash.isEqual(beforeAST, afterAST))) {)?

[mysticatea]

Yes, it's exactly a code I thought.
But I'm wondering we can rewrite it to simple if (!lodash.isEqual(beforeAST, afterAST)) {.
This feature detection might cause to change test results by the existence of util.isDeepStrictEqual.

[platinumazure]

I think the thought was that `util.isDeepStrictEqual` might be faster, but I have no objection to just using lodash.

…

On Tue, Jul 17, 2018, 20:50 Toru Nagashima ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/testers/rule-tester.js <#10612 (comment)>: > @@ -398,6 +398,7 @@ class RuleTester { function assertASTDidntChange(beforeAST, afterAST) { // Feature detect the Node.js implementation and use that if available. + // eslint-disable-next-line node/no-unsupported-features/node-builtins if ((util.isDeepStrictEqual && !util.isDeepStrictEqual(beforeAST, afterAST)) || !lodash.isEqual(beforeAST, afterAST)) { Yes, it's exactly a code I thought. But I'm wondering we can rewrite it to simple if (lodash.isEqual(beforeAST, afterAST)) {. This feature detection might cause to change test results by the existence of util.isDeepStrictEqual. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#10612 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARWepbp3AnIXd5XCM06hAXRMeYV0-ciks5uHpRwgaJpZM4VSzSK> .

[aladdin-add]

util.isDeepStrictEqual was introduced in Node.js 9.0.0 (https://nodejs.org/api/util.html#util_util_isdeepstrictequal_val1_val2)

[mysticatea]

@aladdin-add It's the reason util.isDeepStrictEqual was warned by the node/no-unsupported-features/node-builtins rule.

Problem is:

!util.isDeepStrictEqual(beforeAST, afterAST) || !lodash.isEqual(beforeAST, afterAST)

That || is definitely nonsense. So I hope to fix it, but there are two ways:

1. Ternary operator:

   !(util.isDeepStrictEqual ? util.isDeepStrictEqual(beforeAST, afterAST) : lodash.isEqual(beforeAST, afterAST))

2. Remove the util.isDeepStrictEqual:

   !lodash.isEqual(beforeAST, afterAST)

I prefer the latter because we don't need the forking by feature detection.

[mysticatea]

Anyway, I'll separate that matter to another PR.

[aladdin-add]

I prefer the latter because we don't need the forking by feature detection.

agreed!

[mysticatea]

I sent a PR: #10617

[aladdin-add]

might need to rebase, since #10617 get merged. cc @mysticatea

[mysticatea]

Done!