Update: re-enable experimentalObjectRestSpread (fixes #9990)

[mysticatea]

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

[X] Other, please explain: fixes #9990

What changes did you make? (Give an overview)

1. If parserOptions.ecmaFeatures.experimentalObjectRestSpread is given and parserOptions.ecmaVersion is less than 9, linter enables parserOptions.ecmaVersion: 9 for backward compatibility.
2. If parserOptions.ecmaFeatures.experimentalObjectRestSpread is given, linter generates a deprecation warning to show the deprecation.

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

I make the deprecation warning is as a message with severity: 1. Should it be with another way?

[not-an-aardvark]

Thanks for working on this!

• It seems like it could be useful to print the filename of the config file that contains experimentalObjectRestSpread. As a result, I had been thinking it would be better to put the check somewhere around here (similar to the top-level ecmaFeatures check), rather than in Linter.
• With this check, it seems like a warning would be generated for every file, which seems annoying.

[mysticatea]

Thank you for the good suggestion!

I moved the warning to config-validator and make using process.emitWarning().

Code which enables parserOptions.ecmaVersion: 9 is still in Linter since Linter is a public API.

[not-an-aardvark]

Nitpick: Could this just be return applyDisableDirectives({ // ... again since lintingProblems is no longer being modified afterwards?

[not-an-aardvark]

Nitpick: Could this logic be moved to the resolveParserOptions function?

[not-an-aardvark]

Should we do something like this instead?

function onWarning(w) {
    if (w.code === 'ESLint') {
        warning = w;
    }
}

That way, the test won't break if a warning happens for an unrelated reason (e.g. a deprecation in a dependency).

[not-an-aardvark]

I think the code property of a warning is generally supposed to be unique for a given type of warning, so that tools can filter warnings reliably even if the message changes. Maybe it would be better to do something like { code: 'ESLINT_EXPERIMENTAL_OBJECT_REST_SPREAD' } instead, to make sure it's distinguished from other warnings.

[mysticatea]

I see. Today I leaned the meaning of the code property.

However, I feel reluctant to make code longer since the string will be printed for users. To be clear, Node.js prints this message such as:

(node:10736) [ESLint] DeprecationWarning: The 'parserOptions.ecmaFeatures.experimentalObjectRestSpread' option is deprecated. Use 'parserOptions.ecmaVersion' instead. (found in "tests\fixtures\config-file\experimental-object-rest-spread\extends\common.yml")

The first [ESLint] is the code.

[not-an-aardvark]

Maybe we could make it something like ESLINT_EORS?

[mysticatea]

@not-an-aardvark Thank you for the review! Eventually, I followed your suggestion about the error code.

EDIT: In the following screenshot, I think that this progress bar can surprise contributors. The --no-deprecation flag can disable stderr outputs, but it seems to disable warning events at same time. 🤔

[mysticatea]

https://nodejs.org/api/process.html#process_process_emitwarning_warning_type_code_ctor

This third parameter code doesn't seem to exist in Node.js 6.x 🤔

[aladdin-add]

seems this should use tag Breaking, instead of Update. Hope the bot can check the labels of corresponding issue.