Update: Improves the prefer-object-spread rule by removing extraneous visitors

[sharmilajesupaul]

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

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[X] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

• Includes a change to stop warning if the if the file is importing Object.
• Uses context.getScope() to determine is Object is being overwritten, instead of visiting every assignment and variable declaration node.
• Adds more tests

Is there anything you'd like reviewers to focus on?
There may be a better way to get module and scope information, not sure 🤔

cc. @ljharb @ilyavolodin

[ljharb]

This might collide a bit with #10347

[sharmilajesupaul]

it looks like I can't use object spreads in my test files:
https://github.com/eslint/eslint/pull/10351/files#diff-6088f3cff25518533d89b2e8a4d395faR57

will update.

[g-plane]

Why do you use [].concat instead of using the mapped array directly?

[sharmilajesupaul]

references is an array so the mapped array is a 2-dimensional array with nested arrays of references, the spread with .concat flattens it.

[g-plane]

Maybe we can use Array.prototype.some here.

[platinumazure]

Just requesting two small changes.

Also: I think it would be okay to put parserOptions: { ecmaVersion: 2018, ecmaFeatures: { experimentalObjectRestSpread: true }, sourceType: "module" } in the RuleTester constructor. I don't think there are any tests which need to use sourceType: "script".

[platinumazure]

Would you mind indenting this line and the next line? (Not 100% sure why the indent rule isn't flagging this)

[sharmilajesupaul]

oh weird, good catch

[platinumazure]

This function isn't checking specifically that native Object is being overwritten, right? It just checks that any of the references passed in are being written?

[sharmilajesupaul]

yes it might be better to name it modifyingObjectReference or something

[sharmilajesupaul]

@platinumazure the only test that will fail when using sourceType: module is the test using an html comment. But I can add parser options specifically for that test instead of having to add them for every test using a module.

[platinumazure]

Missed something from last round, sorry, but otherwise everything LGTM. Thanks!

[platinumazure]

Ack, I missed this one. Same indent issue here, would you mind indenting this line and the next?

[ljharb]

This seems like something an automated tool could catch - know of a good one? :-D

[not-an-aardvark]

ref: #8978

[sharmilajesupaul]

I'm a little unclear on what the correct indentation is for this, how is this one supposed to be written? the function code is:

function modifyingObjectReference(references) {
    return references.some(ref => (
        ref.identifier &&
        ref.identifier.parent &&
        isModifier(ref.identifier.parent)
    ));
}

[platinumazure]

Hmm. What I had in mind was:

function modifyingObjectReference(references) {
    return references.some(ref => (
        ref.identifier &&
            ref.identifier.parent &&
            isModifier(ref.identifier.parent)
    ));
}

Looking back, I don't mind this one as much since the lines are already indented from statement position, so nobody will confuse these as potentially multiple statements. If you think this is a reasonable change, please make it; but if not, I can wait for others to review and go with consensus.

[platinumazure]

I've changed my mind-- I don't mind the indent "issue" and if we decide to enforce that in future, we can always run autofix on this codebase later. This LGTM, but I'd like another set of eyes on this.

[btmills]

This is pretty elegant, nice work @sharmilajesupaul!