Skip to content

Add spread rule for iterable objects#264

Merged
tmlayton merged 1 commit into
masterfrom
use-spread
Sep 13, 2017
Merged

Add spread rule for iterable objects#264
tmlayton merged 1 commit into
masterfrom
use-spread

Conversation

@tmlayton

@tmlayton tmlayton commented Aug 11, 2017

Copy link
Copy Markdown
Contributor

Reasons:

Also updated mentions of spread operator to spread syntax, as it is not an operator.

@Shopify/javascript

@gf3

gf3 commented Aug 11, 2017

Copy link
Copy Markdown

not seeing much of a difference in safari, although generally i tend to prefer functions over language syntax due to flexibility and composability

@tmlayton

Copy link
Copy Markdown
Contributor Author

yeah looks like Safari is the only one marginally slower, but both desktop and mobile which applies to web views so Chrome on iOS

@lemonmade

Copy link
Copy Markdown
Contributor

I could mostly go either way on this one, personally. I think I visually prefer Array.from(something), but prefer [...some, more, ...elements], so I dunno. This definitely matches much better to how we do shallow clones of objects (typically, {...otherObject}, although we don't often do it without having more keys to add), but matches slightly worse to other functions that take iterables and convert them to a data structure (new Map(), new Set()). If others feel good about this, I'm on board too ⛵️

@clauderic

Copy link
Copy Markdown
Member

I also personally prefer Array.from() in certain specific cases, for instance when converting a NodeList to an Array, I find Array.from() to be a bit more expressive.

I definitely prefer const clonedArray = [...originalArray] over slice to shallow clone an array.

@amrocha

amrocha commented Aug 16, 2017

Copy link
Copy Markdown

I like the spread syntax a lot, but I don't like the inconsistency in using both spread and Array.from for different kinds of conversions

Since Array.from() covers all cases and spread doesn't I lean towards using Array.from

That being said, if cases where spread doesn't work are fairly rare for us then I'd prefer using spread

Comment thread README.md
```

- [8.4](#8.4) <a name="8.4"></a> Use the spread operator (`...`) to copy arrays, rather than iterating over the array or using `Array#slice`. If you need subsections of the array, continue to use `Array#slice`.
- [8.4](#8.4) <a name="8.4"></a> Use the spread syntax (`...`) to copy arrays, rather than iterating over the array or using `Array#slice`. If you need subsections of the array, continue to use `Array#slice`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@lemonmade lemonmade left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm cool with this 👍 can we also add an issue to make this into a linting rule?

Comment thread README.md Outdated
```

- [8.5](#8.5) <a name="8.5"></a> To convert from an array-like object to an array (for example, a `NodeList` returned by `document.querySelectorAll`, or a jQuery object), use `Array.from`.
- [8.5](#8.5) <a name="8.5"></a> To convert from an array-like object to an array use `Array.from`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just ditch this? I don't think it's particularly relevant advice and it makes the rule feel harder to understand than it really is (just use spread)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favor of ditching this, although spread throws in this example because it has no iteration protocol, e.g.

const arrayLikeObject = {
  0: 'a',
  1: 'b',
  length: 2,

  [Symbol.iterator]() {
    let index = 0;

    return {
      next: () => {
        return this[index]
          ? {value: this[index++], done: false} 
          : {done: true};
      },
    };
  },
};

@perrupa

perrupa commented Aug 18, 2017

Copy link
Copy Markdown
Contributor

I much prefer Array.from() because it is more explicit in what it does. Also, I feel that distinguishing between array-like objects and iterables is going to make this rule much more difficult to understand/follow.

I think the only case I'd favor spread over Array.from() would be when I also want to use a spread anyways.

const newArray= [
  ...nodeList,
  el,
  ...otherArrayLike,
];

Personally, I think spread is for de-structuring and joining arrays, not converting things into them

@tmlayton

Copy link
Copy Markdown
Contributor Author

I removed the Array.from rule for array-like objects and added a comment, similar to 8.4 for Array#slice, to the end of the spread rule:

For objects without an iteration protocol, continue to use Array.from.

And opened #265 to add a linting rule

@tmlayton tmlayton merged commit bbce130 into master Sep 13, 2017
@tmlayton tmlayton deleted the use-spread branch September 13, 2017 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants