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

Added 'Native methods Over Util' article #305

Merged
merged 21 commits into from
Jan 14, 2019

Conversation

Berkmann18
Copy link
Contributor

This PR adds the "Prefer native JS methods over user-land utils like Lodash" article (including the inclusion in the README).

re #302

@goldbergyoni
Copy link
Owner

@sagirk @BrunoScheufler @js-kyle

Since it's the first perf BP in collaboration with a contributor, I'd like to all of your thoughts on how to smoothen this process. Specifically, what should we emphasize in our writing guidelines to minimize back and forth?

@BrunoScheufler What about our linter? 😠 😄

Minor text change- lower case to title case
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@BrunoScheufler BrunoScheufler left a comment

Choose a reason for hiding this comment

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

Looks great so far, I've got some small recommendations to content changes, otherwise good to go!

sections/performance/nativeoverutil.md Outdated Show resolved Hide resolved
sections/performance/nativeoverutil.md Outdated Show resolved Hide resolved
rhapsodyn and others added 4 commits December 29, 2018 10:57
+Removing original eng text
+Fix wired Expression
Co-Authored-By: Berkmann18 <maxieberkmann@gmail.com>
Co-Authored-By: Berkmann18 <maxieberkmann@gmail.com>
@Berkmann18
Copy link
Contributor Author

@BrunoScheufler I applied the changes you requested.

BrunoScheufler
BrunoScheufler previously approved these changes Dec 29, 2018
Copy link
Contributor

@BrunoScheufler BrunoScheufler left a comment

Choose a reason for hiding this comment

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

👍

@BrunoScheufler
Copy link
Contributor

@Berkmann18 thanks for your contribution!

@i0natan I think this is good to go, what do you think? If you agree, you can simply merge it 😄

@Berkmann18
Copy link
Contributor Author

@BrunoScheufler You're welcome 😄.

@goldbergyoni goldbergyoni self-requested a review January 1, 2019 08:09
Copy link
Owner

@goldbergyoni goldbergyoni left a comment

Choose a reason for hiding this comment

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

@Berkmann18 Looks like a promising start and also a piece of valuable advice that is easy to implement. The linter example is a killer 👏

Let's take it 1 level up together, sharpen the messages and learn new things. We might need 1-2 iterations to finalize this bullet.

See my remarks within the text, let me know your thoughts 🙏

README.md Outdated

**TL;DR:** It's often more penalising to use utility libraries like `lodash` and `underscore` over native methods as it leads to unneeded dependencies and slower performance.

**Otherwise:** You'll have to maintain (slightly) bigger projects where you could have simply used what was **already** available or dealt with a few more lines in exchange of a few more files.
Copy link
Owner

Choose a reason for hiding this comment

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

@Berkmann18 Since ~97% of a typical project is 3rd party, I don't think the major downside is the bigger package. Considering that it's also a performance section - I would just emphasize here the degraded performance

Copy link
Contributor Author

@Berkmann18 Berkmann18 Jan 1, 2019

Choose a reason for hiding this comment

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

Sure, I've done that.
Is this:

You'll have to maintain less performant projects where you could have simply used what was already available or dealt with a few more lines in exchange of a few more files.

Fine?

Copy link
Owner

Choose a reason for hiding this comment

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

@Berkmann18 Is this the opening sentence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the sentence for the "Otherwise" section.

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved

### One Paragraph Explainer

Sometimes, using native methods is better than requiring `lodash` or `underscore` because the resulting program will have more dependencies and thus use more space and also not guarantee the best performance all-round.
Copy link
Owner

@goldbergyoni goldbergyoni Jan 1, 2019

Choose a reason for hiding this comment

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

@Berkmann18 I would focus on the performance gain with examples and numbers, this is a performance section. The opening paragraph is right to some extent but should be secondary to the performance improvement idea

Copy link
Contributor Author

@Berkmann18 Berkmann18 Jan 1, 2019

Choose a reason for hiding this comment

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

I've taken that into account and simplified the paragraph which emphasises the performance loss.

sections/performance/nativeoverutil.md Outdated Show resolved Hide resolved
sections/performance/nativeoverutil.md Outdated Show resolved Hide resolved

<br/><br/>

### Code Example – Benchmark test on `_.concat`/`Array.concat`
Copy link
Owner

@goldbergyoni goldbergyoni Jan 1, 2019

Choose a reason for hiding this comment

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

@Berkmann18 Important: This example is very long (see bullet 1 here), we can make the point using more concise examples and visuals:

  • Example: v8 native engine vs Lodash - 4 lines example that shows lodash method and below the v8 alternative
  • Optional: Example: some other title - longer example with more methods
  • Example: benchmark comparison - Lodash vs V8: A graph image showing the performance differences

Copy link
Owner

Choose a reason for hiding this comment

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

@Berkmann18 At your own convenience, try thinking how to shorten and sharpen this example. I know how hard it's, found myself more than a few times trying to cut in a half a complex code sample

Copy link
Contributor Author

@Berkmann18 Berkmann18 Jan 4, 2019

Choose a reason for hiding this comment

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

I found a way to shorten that example without hiding too much (say, if someone were to copy-paste the code without missing necessary bits).
And I kept the _.concat code example while adding a section with a graph on the Lodash vs V8 example you gave.

sections/performance/nativeoverutil.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Improved the examples and swapped some text with images.
Copy link
Owner

@goldbergyoni goldbergyoni left a comment

Choose a reason for hiding this comment

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

@Berkmann18 It's now even better, the graphs and the examples are concise and sharper 🎖

Le'ts have a last short iteration, see my comments, I'm tuned to approve it once you flag that the changes are ready

Most of the issues are formatting issues, it's not consistent enough with how other bullets look like, kindly have a look at other quotes/code-examples and try to unify

README.md Show resolved Hide resolved
sections/performance/nativeoverutil.md Outdated Show resolved Hide resolved
### Example: benchmark comparison - Lodash vs V8 (Native)
The graph below shows the mean of the benchmarks for a variety of Lodash methods, this shows that Lodash methods take on average 146.23% more time to complete the same tasks as V8 methods.

![meanDiag](../../assets/images/sampleMeanDiag.png)
Copy link
Owner

Choose a reason for hiding this comment

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

Let's include a link to the article?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a link to the relevant spreadsheet if that's fine?

sections/performance/nativeoverutil.md Outdated Show resolved Hide resolved
sections/performance/nativeoverutil.md Outdated Show resolved Hide resolved

> Lodash and Underscore are great modern JavaScript utility libraries, and they are widely used by Front-end developers. However, when you are targeting modern browsers, you may find out that there are many methods which are already supported natively thanks to ECMAScript5 [ES5] and ECMAScript2015 [ES6]. If you want your project to require fewer dependencies, and you know your target browser clearly, then you may not need Lodash/Underscore.

There's also an [ESLint plugin](https://www.npmjs.com/package/eslint-plugin-you-dont-need-lodash-underscore) which detects where you're using libraries but don't need to.
Copy link
Owner

Choose a reason for hiding this comment

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

Create a new section, new paragraph, new Example: Linting for non-native methods usage. Separate this from the quote @Berkmann18

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I've revisited in that part and the example below that part to make it coherent.

@goldbergyoni
Copy link
Owner

@Berkmann18 Perfect. A star will be assigned by the end of January #318 🏅

I will also Tweet this new BP today.

Hope to see you here again soon :]

@goldbergyoni goldbergyoni merged commit 3933767 into goldbergyoni:master Jan 14, 2019
@Berkmann18
Copy link
Contributor Author

Woo! I'm glad I contributed and look forward to contributing more.

elite0226 pushed a commit to elite0226/nodebestpractices that referenced this pull request Oct 31, 2022
Added 'Native methods Over Util' article
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.

None yet

7 participants