-
Notifications
You must be signed in to change notification settings - Fork 37
Add spread rule for iterable objects #264
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
Conversation
|
not seeing much of a difference in safari, although generally i tend to prefer functions over language syntax due to flexibility and composability |
|
yeah looks like Safari is the only one marginally slower, but both desktop and mobile which applies to web views so Chrome on iOS |
|
I could mostly go either way on this one, personally. I think I visually prefer |
|
I also personally prefer I definitely prefer |
|
I like the spread syntax a lot, but I don't like the inconsistency in using both spread and Since That being said, if cases where spread doesn't work are fairly rare for us then I'd prefer using spread |
| ``` | ||
|
|
||
| - [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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
lemonmade
left a comment
There was a problem hiding this comment.
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?
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`. |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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};
},
};
},
};|
I much prefer I think the only case I'd favor spread over const newArray= [
...nodeList,
el,
...otherArrayLike,
];Personally, I think spread is for de-structuring and joining arrays, not converting things into them |
370cc43 to
df739a3
Compare
df739a3 to
504c812
Compare
|
I removed the
And opened #265 to add a linting rule |
Reasons:
NodeList, a test with jQuery object, and a chart) but we are talking high operations a second so not sure it is a noticeable differenceAlso updated mentions of spread operator to spread syntax, as it is not an operator.
@Shopify/javascript