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

New: Adds prefer-object-spread rule (refs: #7230) #9955

Merged
merged 10 commits into from May 11, 2018

Conversation

Projects
None yet
6 participants
@sharmilajesupaul
Copy link
Contributor

sharmilajesupaul commented Feb 6, 2018

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

What changes did you make? (Give an overview)
This is a follow up to #7230, we implemented prefer-object-spread internally at @airbnb.
The rule requires that you use an object spread over an Object.assign call with multiple arguments and an object literal as the first argument.

We also warn on cases where an Object.assign call is made using a single argument that is an object literal, in this case, the Object.assign is not needed (7f7f97d).

The following patterns are considered errors:

Object.assign({}, foo)

Object.assign({}, {foo: 'bar'})

Object.assign({ foo: 'bar'}, baz)

Object.assign({ foo: 'bar' }, Object.assign({ bar: 'foo' }))

Object.assign({}, { foo, bar, baz })

Object.assign({}, { ...baz })

// Object.assign with a single argument that is an object literal
Object.assign({});

Object.assign({ foo: bar });

The following patterns are not errors:

Object.assign(...foo);

// Any Object.assign call without an object literal as the first argument
Object.assign(foo, { bar: baz });

Object.assign(foo, Object.assign({ bar: 'foo' }));

Object.assign(foo, { bar, baz })

Object.assign(foo, { ...baz });

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

I made 2 commits in this PR

  1. 6e3e035 handles the general use case where Object.assign is called using an object literal and an additional argument, which can be changed to use object spread.
  2. 7f7f97d warns on cases where Object.assign has a single argument that is an object literal. In this case, you can use the object literal directly. (i.e. Object.assign({}) -> {})

I left the second part in its own commit because I wasn't sure if it would fit into the scope of this rule.

cc. @ljharb

@eslint eslint bot added the triage label Feb 6, 2018

@sharmilajesupaul sharmilajesupaul changed the title New: Adds prefer-object-spread rule (refs: #7230) New: Adds prefer-object-spread rule (refs: #7230) Feb 6, 2018

@mysticatea
Copy link
Member

mysticatea left a comment

Thank you for the contribution!!
Looks nice to me, but I left some requests.

About 7f7f97d, I'm not sure if it should be a part of this rule, but I don't oppose it.

) {
context.report({
node,
message:

This comment has been minimized.

@mysticatea

mysticatea Feb 7, 2018

Member

Would you use messageId instead message?

{
code: "Object.assign(foo, { bar: baz })",
parserOptions
}

This comment has been minimized.

@mysticatea

mysticatea Feb 7, 2018

Member

Would you add tests for ecmaVersion: 2018?


create: function rule(context) {
return {
CallExpression: node => {

This comment has been minimized.

@mysticatea

mysticatea Feb 7, 2018

Member

Would you use a consistent way for create and CallExpression (e.g., method shorthand)?

I wonder if we have enabled object-shorthand rule in our codebase. 🤔

docs: {
description:
"disallow using Object.assign with an object literal as the first argument and prefer the use of object spread instead.",
category: "ECMAScript 6",

This comment has been minimized.

@mysticatea

mysticatea Feb 7, 2018

Member

Would you change the category to Stylistic Issues?

ES6 doesn't look proper category. Though it has not done, #7991 has been accepted.

type: "CallExpression"
}
]
}

This comment has been minimized.

@mysticatea

mysticatea Feb 7, 2018

Member

Would you add tests for comments and parentheses? I expect comments to be kept.

For example:

Object.assign(
    {},
    // a comment
    foo,
    // another comment
    { a: true }
)
Object.assign(
    {},
    ({ a }),
    b ? c : { d },
    (e, f)
)
* @returns {Function} autofixer - replaces the Object.assign with a spread object.
*/
function autofixSpread(node, sourceCode) {
return fixer => {

This comment has been minimized.

@mysticatea

mysticatea Feb 7, 2018

Member

I guess this might be able to simplify with generator function. (It's just a impression)

This comment has been minimized.

@sharmilajesupaul

sharmilajesupaul Apr 13, 2018

Author Contributor

could you give me an example of this? I'm not too familiar with generator functions.


When Object.assign is called using an object literal the first argument, this rule requires using the object spread syntax instead. This rule also warns on cases where an `Object.assign` call is made using a single argument that is an object literal, in this case, the `Object.assign` call is not needed.

**Please note:** This rule can only be used when using an `ecmaVersion` of 2018 or higher, 9 or higher, or when using an `ecmaVersion` of 2015-2017 or 5-8 with the `experimentalObjectRestSpread` parser option enabled.

This comment has been minimized.

@mysticatea

mysticatea Feb 7, 2018

Member

I think that the note about experimentalObjectRestSpread doesn't need since the option would be removed in nearly future.

@mysticatea mysticatea added rule accepted feature and removed triage labels Feb 7, 2018

@sharmilajesupaul

This comment has been minimized.

Copy link
Contributor Author

sharmilajesupaul commented Feb 8, 2018

Thanks for the comments @mysticatea! I'll get to these tomorrow or Saturday at the latest, just fyi.

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Mar 22, 2018

Hi @sharmilajesupaul, just wanted to follow up and ask if this is ready for another round of review or if there is more to be done here. Thanks!

@sharmilajesupaul

This comment has been minimized.

Copy link
Contributor Author

sharmilajesupaul commented Mar 22, 2018

@platinumazure yes yes wow I completely forgot, sorry! I spent a bunch of time addressing multiline objects that warn on this rule and never updated my PR, I will update it today

@sharmilajesupaul

This comment has been minimized.

Copy link
Contributor Author

sharmilajesupaul commented Mar 23, 2018

Made a big change to the fixer to work with multiline objects. Now it spreads object literal arguments instead of spreading them. Also addressed a majority of last code review requests. I haven't worked with generators much, so I'm not sure how that will work for the fixer? cc. @ljharb @platinumazure @mysticatea

@platinumazure
Copy link
Member

platinumazure left a comment

Requesting some small documentation changes. Haven't had time to review the code/tests yet, but I hope to do so in the next few days. Thanks!

@@ -0,0 +1,47 @@
# Prefer use of an object spread over `Object.assign` (prefer-object-spread)

When Object.assign is called using an object literal the first argument, this rule requires using the object spread syntax instead. This rule also warns on cases where an `Object.assign` call is made using a single argument that is an object literal, in this case, the `Object.assign` call is not needed.

This comment has been minimized.

@platinumazure

platinumazure Mar 23, 2018

Member

Small typo: "using an object literal the first argument" --> "using an object literal as the first argument".

Also, we generally prefer that the first section should only have background about object spread and Object.assign-- the Rule Details section is where the rule's purpose/method should be outlined.


## Rule Details

The following patterns are considered errors:

This comment has been minimized.

@platinumazure

platinumazure Mar 23, 2018

Member

Could this be replaced with:

Examples of **incorrect** code for this rule:

Thanks!

Object.assign({ foo: bar });
```

The following patterns are not errors:

This comment has been minimized.

@platinumazure

platinumazure Mar 23, 2018

Member

Could this be replaced with:

Examples of **correct** code for this rule:

Thanks!

sharmilajesupaul added some commits Feb 6, 2018

Adds exception for `Object.assign` with one object literal argument
This was an exception to this rule that we use internally, in the case that an
`Object.assign` call is made with an object literal as the only argument. The
`Object.assign` call is not needed and we can just use the object literal
directly.
Many fixes and improvements:
- handles nested object literals
- handles various comment types
- places comma after arguments in a smarter way

@sharmilajesupaul sharmilajesupaul force-pushed the sharmilajesupaul:shar--prefer-object-spread branch from 97c0127 to 3819ad4 Apr 13, 2018

@sharmilajesupaul

This comment has been minimized.

Copy link
Contributor Author

sharmilajesupaul commented Apr 13, 2018

Just pushed updates to this PR:

Sorry it took so long!

cc @platinumazure, @mysticatea, @ljharb

@ljharb

ljharb approved these changes Apr 13, 2018

Copy link
Contributor

ljharb left a comment

LGTM - all the spacing seems reasonable and adjustable via other rules, and using parens wherever it's not known to be safe (because the object literal might be a block) seems the right call.

@platinumazure
Copy link
Member

platinumazure left a comment

Finally had time to review this in full (thanks for stopping by the Gitter chat). This is looking great on the whole, love the test cases (including coverage on comments, even HTML comments).

I left a few minor questions and nitpicks. I'm fairly confident at least one or two of them are non-contentious so I've labeled this review as Request Changes.

Thanks for all your hard work (and patience!) on this.

// Any Object.assign call without an object literal as the first argument
Object.assign(foo, { bar: baz });
Object.assign(foo, Object.assign({ bar: 'foo' }));

This comment has been minimized.

@platinumazure

platinumazure Apr 24, 2018

Member

Question: Should the inner (second) Object.assign be flagged here? If so, would it be better to add a non-literal first argument so the whole line is a correct example?

This comment has been minimized.

@sharmilajesupaul

sharmilajesupaul Apr 29, 2018

Author Contributor

yeah it should good catch

*/
function addComma(arg) {
const nonWhitespaceCharacterRegex = /[^\s\\]/g;
const commentRegex = /(\/\*[\w'\s\r\n*]*\*\/)|(\/\/[\w\s']*)|(<![-\-\s\w>/]*>)/g;

This comment has been minimized.

@platinumazure

platinumazure Apr 24, 2018

Member

Rather than taking a string and matching for comments, would it make more sense to use token/comment retrieval methods in SourceCode? I think this basically boils down to getting the last non-comment token in the argument node, and adding a comma afterward.

This comment has been minimized.

@sharmilajesupaul

sharmilajesupaul Apr 24, 2018

Author Contributor

Thank you for mentioning this. Instead of the regex, we can just use the ranges in context.getCommentsInside(node). Definitely over complicated this a bit 😬

const nonWhitespaceCharacters = Array.from(matchAll(arg, nonWhitespaceCharacterRegex));
const commentRanges = [];

// Create a ranges of starting and ending indicies for comments found

This comment has been minimized.

@platinumazure

platinumazure Apr 24, 2018

Member

Nit: s/indicies/indices

create: function rule(context) {
return {
CallExpression(node) {
const sourceCode = context.getSourceCode();

This comment has been minimized.

@platinumazure

platinumazure Apr 24, 2018

Member

Nit: You could move this a few lines up so that we don't need to retrieve the SourceCode object on every CallExpression. (It's a persistent reference.)

"let a = Object.assign(a, b)",
"Object.assign(a, b)",
"let a = Object.assign(b, { c: 1 })",
"let a = Object.assign({}, ...b)",

This comment has been minimized.

@platinumazure

platinumazure Apr 24, 2018

Member

Should this be valid? Wondering if the correct code should be let a = Object.assign(...b);, or am I missing something? (Compare with the test case 2 lines below this one)

This comment has been minimized.

@ljharb

ljharb Apr 24, 2018

Contributor

Statement and expression positions differ in terms of how the autofixer outputs parens, but good call, this one should be a spread.

]
},

// TODO: Handle nested Object.assign calls

This comment has been minimized.

@platinumazure

platinumazure Apr 24, 2018

Member

Should this comment be removed? (Although, now I see some of the test cases below, seems there might be more to do here?)

errors: [
{
messageId: "useSpreadMessage",
type: "CallExpression"

This comment has been minimized.

@platinumazure

platinumazure Apr 24, 2018

Member

Could this error (and the one below) have more information to help identify where the errors are reported? (e.g., line/column)

{
code:
"Object.assign({ foo: 'bar' }, Object.assign({ bar: 'foo' }, Object.assign({}, { superNested: 'butwhy' })))",
output: "({foo: 'bar', ...Object.assign({ bar: 'foo' }, Object.assign({}, { superNested: 'butwhy' }))})",

This comment has been minimized.

@platinumazure

platinumazure Apr 24, 2018

Member

I think the innermost Object.assign should be identified and fixed-- is that a known limitation at this point? Or are we relying on multipass autofix to catch these cases? (Now the TODO above makes more sense...)

This comment has been minimized.

@ljharb

ljharb Apr 24, 2018

Contributor

Multipass, i think.

This comment has been minimized.

@sharmilajesupaul

sharmilajesupaul Apr 29, 2018

Author Contributor

I had a solution earlier that would recurse through nested object.assign calls, but the current implementation relies on multipass

errors: [
{
messageId: "useSpreadMessage",
type: "CallExpression"

This comment has been minimized.

@platinumazure

platinumazure Apr 24, 2018

Member

Could this error (and the ones below) have more information, e.g. line/column numbers?

Several improvements:
- adds line and column numbers to tests
- warn on cases where argument is a spread element,
- use getCommentsInside instead of regex
- fix example in doc

@sharmilajesupaul sharmilajesupaul force-pushed the sharmilajesupaul:shar--prefer-object-spread branch from 97bab19 to 7a7960a May 7, 2018

@sharmilajesupaul

This comment has been minimized.

Copy link
Contributor Author

sharmilajesupaul commented May 7, 2018

@platinumazure ptal, made improvments in 7a7960a

@platinumazure
Copy link
Member

platinumazure left a comment

Left one comment inline.

In addition, I'm not sure about the Object.assign({ objLiteral: "value" }) case. There's no check (that I can see) which checks that this is actually Object.assign. Could you please add a test foo({ bar: "baz" }) and show that this is not reported?

Also, I'm not sure how the case where Object is overwritten is handled: const Object = {}; Object.assign({ foo: "bar" }); (ideally should not report). I don't think this should block the release of this feature for now, though-- we can fix later.

schema: [],
fixable: "code",
messages: {
useSpreadMessage: "Use an object spread instead of `Object.assign()` eg: `{ ...foo }`",

This comment has been minimized.

@platinumazure

platinumazure May 10, 2018

Member

I don't think the example should be necessary. Users can view the documentation (or search the Internet) for what object spread means.

This comment has been minimized.

@ljharb

ljharb May 10, 2018

Contributor

It's much more helpful to get the warning in-editor, though :-/ is there any reason the example can't be included?

This comment has been minimized.

@platinumazure

platinumazure May 10, 2018

Member

Well, turning that around, why doesn't the object literal case have an example?

Also: Since we have autofix here, the user could just let that be run and see how the item should look. (And no, we don't need to note that --fix could be run in the lint message, because we note that in most formatters outside of the lint message)

This comment has been minimized.

@ljharb

ljharb May 10, 2018

Contributor

I suppose that's a fair point - maybe it should have an example :-)

(also since autofix can't be easily run for a single rule, that's not really a practical option for demonstrating the results of a single rule)

Check if the native `Object` is being overwritten, if it is, do not w…
…arn.

Bug fix to ensure that an we're warning on an `Object.assign` for the literal case.
@sharmilajesupaul

This comment has been minimized.

Copy link
Contributor Author

sharmilajesupaul commented May 11, 2018

@platinumazure there should have been a line in the object literal case, to make sure that it was only warning on Object.assign, not sure how I missed that. But I added it back in along with the test case:
751774d#diff-c7ab2d044a2d5cc255c07ea6116488e1R267

I also updated the rule to pick up on whether the native Object is overwritten in a variable declaration or assignment in the current file. 751774d#diff-c7ab2d044a2d5cc255c07ea6116488e1R210

@not-an-aardvark
Copy link
Member

not-an-aardvark left a comment

Sorry about the late review, I just had one question about whether it's necessary to add a dependency.

*/
function addComma(formattedArg, comments) {
const nonWhitespaceCharacterRegex = /[^\s\\]/g;
const nonWhitespaceCharacters = Array.from(matchAll(formattedArg, nonWhitespaceCharacterRegex));

This comment has been minimized.

@not-an-aardvark

not-an-aardvark May 11, 2018

Member

Is it necessary to use matchAll here? Since this regex doesn't have any capturing groups, it seems like this could just be done with:

const nonWhitespaceCharacters = formattedArg.match(nonWhitespaceCharacterRegex);

That would avoid the need to add a dependency on string.prototype.matchall.

This comment has been minimized.

@ljharb

ljharb May 11, 2018

Contributor

With a global regex, .match does not return an array of match objects, so there's no way to get the index (position) of each match - this is the entire reason .matchAll is needed in the language.

So, no, I don't think there's any way to avoid the dependency (without inlining it, of course, which would be a terrible idea)

@ilyavolodin ilyavolodin merged commit 1a6b399 into eslint:master May 11, 2018

5 checks passed

commit-message PR title follows commit message guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details

FloEdelmann added a commit to FloEdelmann/codacy-eslint that referenced this pull request Sep 14, 2018

@eslint eslint bot locked and limited conversation to collaborators Nov 8, 2018

@eslint eslint bot added the archived due to age label Nov 8, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.