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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JavaScript Guide: Arrays #35

Merged
merged 12 commits into from
Nov 7, 2018
Merged

Conversation

gerardo-rodriguez
Copy link
Member

@gerardo-rodriguez gerardo-rodriguez commented Nov 1, 2018

Summary

This PR adds the "Arrays" section.

Use the GH UI as shown in the GIF below to view the rendered README:

view-file

@gerardo-rodriguez gerardo-rodriguez self-assigned this Nov 1, 2018
@gerardo-rodriguez gerardo-rodriguez requested review from a team November 1, 2018 00:24

### 3.6 Mapping Over Iterables

Use [array spread syntax][Array Literal Spread Syntax] `...` instead of [`Array.from`][Array.from] for mapping over iterables.
Copy link
Member Author

Choose a reason for hiding this comment

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

I ran a JSPerf and it seems going with the opposite of what the Airbnb Guide suggests performs better overall. I'd like your thoughts, if you have any, on this. I wanted to make sure my JSPerf tests were accurate and that I wasn't testing incorrectly.

Copy link
Member

Choose a reason for hiding this comment

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

I tried it in Chrome 69 and Chrome 70 on Linux, which resulted in spread being more than twice as fast, and I also tried FF on Linux, where spread is almost twice as fast.

Copy link
Member

Choose a reason for hiding this comment

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

I also created this test, which compared Array.from() and [...].map() as well as Array.from().map. https://jsperf.com/array-from-vs-spread-vs-array-from-map/1

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice! Thanks for the test! I think I'll add that to the docs! 👍

Copy link
Contributor

@Josh68 Josh68 Nov 1, 2018

Choose a reason for hiding this comment

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

Interesting. On this note, is there any thought of adding comments about native support (thinking browsers), so that we don't end up relying on Babel or polyfills to use what should be more performant, future-friendly methods, but are actually being transpiled to something less performant, at least for some clients.

I realize this is a moving target and also very much a case-by-case thing, but even links to caniuse might be helpful.

Copy link
Member Author

@gerardo-rodriguez gerardo-rodriguez Nov 1, 2018

Choose a reason for hiding this comment

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

Great idea @Josh68! I added GH issue to add caniuse.com links. Maybe I'll sneak them in as I go. If not, that GH issue will give us the opportunity to do so.

@gerardo-rodriguez gerardo-rodriguez changed the title JavaScript Guide: Arrays (Part 1) JavaScript Guide: Arrays Nov 1, 2018

- ESLint: [array-callback-return]

### 3.8 Array Line Breaks
Copy link
Member Author

Choose a reason for hiding this comment

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

@calebeby @Josh68 Please also pay special attention to this section. I know we have some rules in place but I don't know if what I show here is accurate. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

It appears that the rule is that line breaks, indents, etc are driven by the individual contents of the array, not the array itself, which defaults to no formatting (i.e., inline). https://eslint.org/docs/rules/array-element-newline and related rulesets apply, but I'm not sure what the best settings are.

Copy link
Member

Choose a reason for hiding this comment

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

Like I said in the strings PR, can we just have something like:

Code should conform to Prettier formatting

instead of concerning ourselves with individual stylistic details?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm tempted to remove the 3.8 Array Line Breaks section. I'm not sure how helpful it would be to have "Code should conform to Prettier formatting".

Do you feel strongly otherwise, @calebeby? 🤔I'm open to be convinced if you feel there's a benefit. :)

Copy link
Member

Choose a reason for hiding this comment

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

I think you should mention prettier somewhere in the js guide, but I think it is fine if you remove that section and put the prettier note at the top or something.

Copy link
Member

@tylersticka tylersticka left a comment

Choose a reason for hiding this comment

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

I will leave the specifics to debate among those in @cloudfour/dev, but the formatting of this is now much more readable and I could easily understand it.

@gerardo-rodriguez
Copy link
Member Author

@calebeby @Josh68 This PR has been updated and is available for review again. Thanks!

Copy link
Member

@calebeby calebeby left a comment

Choose a reason for hiding this comment

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

:shipit:

@gerardo-rodriguez gerardo-rodriguez merged commit e3d7165 into master Nov 7, 2018
@gerardo-rodriguez gerardo-rodriguez deleted the feature/js-guide-arrays-part-1 branch November 7, 2018 00:11
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.

4 participants