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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃帀 Object Rest/Spread Properties arrived at stage 4 #9885

Comments

@mysticatea
Copy link
Member

@mysticatea mysticatea commented Jan 25, 2018

tc39/proposals@7a04292

It's the time to update our rules to be according to the current ESTree spec.

  • Update Espree to support the syntax in ecmaVersion:2018.
    I think we should keep ecmaFeatures.experimentalObjectRestSpread to avoid breaking changes.
    1. If ecmaVersion:2018 and no ecmaFeatures.experimentalObjectRestSpread, Espree generates RestElement/SpreadElement.
    2. If ecmaFeatures.experimentalObjectRestSpread:true, Espree generates ExperimentalRestProperty/ExperimentalSpreadProperty as is.
  • Update core rules to support the syntax.
@ljharb

This comment has been minimized.

Copy link
Contributor

@ljharb ljharb commented Jan 25, 2018

Any chance we could reopen #7230 now? :-D

(Separately, you might want to make 2018, or 2019 if 2018's ship has sailed, a schema error when combined with experimentalObjectRestSpread)

@not-an-aardvark

This comment has been minimized.

Copy link
Member

@not-an-aardvark not-an-aardvark commented Jan 25, 2018

I made a list awhile ago of things that need to be updated:

  • Everything that checks for ExperimentalRestProperty and ExperimentalSpreadProperty will need to check for RestElement and SpreadElement instead.
    • comma-dangle
    • no-self-assign
    • rest-spread-spacing
    • object-shorthand
    • no-unused-vars
  • no-dupe-keys should maybe check duplicate spread elements
@ljharb

This comment has been minimized.

Copy link
Contributor

@ljharb ljharb commented Jan 25, 2018

Duplicate spread elements may not be an error; Object.assign({}, { a: undefined }, { a: 1 }) (where undefined is a variable value) is a legit defaulting pattern.

@not-an-aardvark

This comment has been minimized.

Copy link
Member

@not-an-aardvark not-an-aardvark commented Jan 25, 2018

I'm referring to objects like ({ ...foo, ...foo }). Unless foo is doing weird things with getters, it doesn't seem like it would be useful to spread the same value twice in an object, because the first set of properties would get completely overwritten by the second set of properties.

@ljharb

This comment has been minimized.

Copy link
Contributor

@ljharb ljharb commented Jan 25, 2018

In that case, foo isn't a key, so it wouldn't be appropriate for no-dupe-keys to warn on it. A new "no-duplicate-spreaded-objects` rule might cover it though :-)

@not-an-aardvark

This comment has been minimized.

Copy link
Member

@not-an-aardvark not-an-aardvark commented Jan 25, 2018

foo isn't a key itself, but the keys of foo end up duplicated in the object when foo is spread twice. Code like this:

const foo = { a: 1, b: 2 };
({
    ...foo,
    ...foo
});

...is effectively doing the same thing as code like this:

({
    a: 1, b: 2,
    a: 1, b: 2
})

The latter is covered by no-dupe-keys, so I think it would be reasonable to flag the former too.

@ljharb

This comment has been minimized.

Copy link
Contributor

@ljharb ljharb commented Jan 25, 2018

Sure, but you could only do that when foo is in scope in the file (altho that seems useful when it's possible)

@not-an-aardvark

This comment has been minimized.

Copy link
Member

@not-an-aardvark not-an-aardvark commented Jan 25, 2018

Even if foo is some unknown collection of keys, we can still determine the existence of duplicate keys1 when we see ({ ...foo, ...foo }) because whatever keys are in foo will be duplicated, although we don't know what the keys are at linting time.

1 I suppose there wouldn't be duplicate keys if foo is always an empty object, but it doesn't seem useful to spread an always-empty object anyway.

@platinumazure

This comment has been minimized.

Copy link
Member

@platinumazure platinumazure commented Jan 25, 2018

Maybe we could create a new rule (e.g., no-duplicate-spread) for the spread case? That way users can decide how important it is and we don't need to try to justify if those are keys or not.

@mysticatea mysticatea added accepted and removed evaluating labels Jan 25, 2018
@ljharb

This comment has been minimized.

Copy link
Contributor

@ljharb ljharb commented Jan 25, 2018

As you said, though, if the object has getters, then duplicate keys might be desired so that it invokes the getter multiple times.

@not-an-aardvark

This comment has been minimized.

Copy link
Member

@not-an-aardvark not-an-aardvark commented Jan 25, 2018

I don't have a problem with ESLint getting confused if someone is using object spread for the purpose of repeatedly invoking a getter with side-effects, because anyone reading that code will probably be confused too.

@platinumazure

This comment has been minimized.

Copy link
Member

@platinumazure platinumazure commented Jan 25, 2018

@ljharb A similar argument could be made for no-dupe-keys with getters on the keys. The recommendation in both cases is the same: Use inline disable comments judiciously, or don't use the rule. We can't expect rules to account for all possible edge cases. 馃槃

@ilyavolodin

This comment has been minimized.

Copy link
Member

@ilyavolodin ilyavolodin commented Jan 25, 2018

That's the case for a most of ESLint rules. Getters and setters are just not something that can be statically analyzed, so we choose to not try. A lot of ESLint rules will fail when used with getters that return different values.

@mysticatea

This comment has been minimized.

Copy link
Member Author

@mysticatea mysticatea commented Feb 5, 2018

I'm working on this for existing rules.

mysticatea added a commit that referenced this issue Feb 5, 2018
@mikermcneil

This comment has been minimized.

Copy link

@mikermcneil mikermcneil commented Feb 9, 2018

First, just a data point on usage: We're all in on eslint now as of Sails v1, and I've really appreciated how specific rules have been, and the continued focus on that for new ES functionality. Bravo! Since Node incorporates this as of Node 8.6, we're going ahead generating new projects with experimentalObjectRestSpread: true in the .eslintrc for their server-side code.

@ljharb 2垄 re: duplicates: I do think you're right about the duplicate spread symbol....thingies. On one hand, I could see someone considering using something like x = {...x, ...y, ...x} to do what you might accomplish with x = Object.assign({}, y, x) today, and I guess there is some nuance you could achieve as far as key precedence. But it still seems like a code smell to me-- at least with the example code being discussed-- since it obfuscates what's going on, making it look like there's mutation to worry about.

So anyway, it might be worthwhile to call this out with a distinct rule. Besides, for every separate rule eslint contains, the better (and more productive) of an experience developers have when working with eslint integration in their text editor (i.e. because the error messages displayed are more precise and thus helpful)

mikermcneil added a commit to balderdashy/sails-generate that referenced this issue Feb 9, 2018
mikermcneil added a commit to sailshq/sails-hook-organics that referenced this issue Feb 9, 2018
This was referenced Mar 22, 2018
@eslint eslint bot locked and limited conversation to collaborators Aug 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can鈥檛 perform that action at this time.