Update style guide for ES6 by bajtos · Pull Request #187 · loopbackio/loopback.io

bajtos · 2016-12-05T11:26:22Z

Now that we have dropped support for pre-v4 versions of Node, it's time to start using ES6 goodness in our code base. In this patch, I did:

Copy the existing guide to style-guide-es5
Add few ES6-specific sections
Update all code examples to use the new ES6 syntax

@superkhau @strongloop/loopback-dev PTAL

@sam-github @bnoordhuis Since you are contributing to Node core, which has switched to ES6 long time ago, I think you can help us with avoiding mistakes others have already learnt from. Are any parts of my proposal different from current best practices in Node core? Is there any other ES6-related best practices to add to our style guide?

- Copy the existing guide to style-guide-es5 - Add few ES6-specific sections - Update all code examples to use the new ES6 syntax

bnoordhuis · 2016-12-05T11:29:25Z

pages/en/contrib/style-guide.md

+
+if (customer.isPreferred) {
+  discount = 0.1;
+} else if (customer.address.state = 'CA') {


== or ===?

bnoordhuis · 2016-12-05T11:29:46Z

pages/en/contrib/style-guide.md

+    return 0.1;
+  }
+
+  if (customer.address.state = 'CA') {


bnoordhuis · 2016-12-05T11:30:22Z

pages/en/contrib/style-guide.md

+arrow function or a regular function:
+
+ - Arrow function preserves `this` from the outer scope. This is much more
+   perfomant than the usual workaround of storing `this` in a `self` variable.


Typo: performant. I personally hate that word though. :-)

bnoordhuis · 2016-12-05T11:31:03Z

pages/en/contrib/style-guide.md

+
+### Classes
+
+ES6 introduced syntax-sugar for defining classes, start using its' advantages


Typo: its, no apostrophe.

bnoordhuis · 2016-12-05T11:32:01Z

pages/en/contrib/style-guide.md

+{% include important.html content="
+
+The rules described in this document are evolving, therefore not all of our
+code-base is following them yet. This is expected and OK. Our approach is to


s/is following/are following/

bnoordhuis · 2016-12-05T11:32:59Z

pages/en/contrib/style-guide.md

+
+### Variable declarations
+
+Prefer to use immutable variables declared with `const` keyword:


Might be worth pointing out that const makes the reference immutable but not the referenced object. It's something that seems to trip up a lot of people.

bnoordhuis · 2016-12-05T11:34:16Z

pages/en/contrib/style-guide.md

+
+```js
+const user = app.models.User;
+const saltWorkFactor = user.settings.saltWorkFactor;


Arguably even better: const { saltWorkFactor } = user.settings;

Aside: I believe user should be User?

bnoordhuis · 2016-12-05T11:36:50Z

pages/en/contrib/style-guide.md

+```
+
+In the rare cases where you need to split variable declaration and
+initialization, prefer to use `let` over `var`:


let currently has a small caveat in that using it in loop constructs causes V8 to deoptimize the function, i.e., for now you should write:

for (var i = 0; i < 42; ++i) doit(i);

Instead of:

for (let i = 0; i < 42; ++i) doit(i);

This will be fixed in node.js v8.x but probably won't be fixed in v4.x and v6.x.

bnoordhuis · 2016-12-05T11:36:59Z

pages/en/contrib/style-guide.md

+
+### Arrow functions
+
+The are two considerations to keep in mind when deciding whether to use an


bnoordhuis · 2016-12-05T11:38:09Z

pages/en/contrib/style-guide.md

+   before/after hooks, then you cannot use an arrow function.
+
+ - Arrow functions are anonymous, there is no easy way for assigning them a
+   descriptive name. This makes it difficult to understand stack traces.


Half-true. V8 does a fairly good job of inferring a name when you assign them to variables.

bajtos · 2016-12-05T11:54:55Z

@bnoordhuis lovely ❤️ , thank you for your feedback! I applied all your comments in 0106dea, would you like to take another look?

crandmck · 2016-12-05T17:36:01Z

Would you also like me to add this to the "Contributing to LB" navigation - http://loopback.io/doc/en/contrib/code-contrib.html ? Easy enough to do - just LMK.

superkhau · 2016-12-06T09:02:49Z

Great feedback, thanks @bnoordhuis and @bajtos for updating. LGTM.

bajtos · 2016-12-06T09:25:19Z

Would you also like me to add this to the "Contributing to LB" navigation - http://loopback.io/doc/en/contrib/code-contrib.html ? Easy enough to do - just LMK.

The "code-contrib.html" already contains a link to "style-guide.md" AFAICT, see the bullet point "Adhere to code style outlined in the style guide.". (I updated an existing file in this patch, so that's expected.)

Perhaps you are asking whether to add "style-guide-es5.md" to that document? I think that's not necessary, since most back-porting work is done by us, not the community.

@superkhau thoughts?

eddiemonge · 2016-12-06T17:08:14Z

pages/en/contrib/style-guide.md

+    // ...
+  }
+
+ set(modelName, key, value, options, callback) {


indentation is off here

Good catch, fixed in bf65d6b

superkhau · 2016-12-06T18:17:33Z

Perhaps you are asking whether to add "style-guide-es5.md" to that document? I think that's not necessary, since most back-porting work is done by us, not the community.

I agree, we don't want more burden of maintaining ES5 docs in addition to ES6. That said though, is there a way to "freeze" the old ES5 docs with LB2 @crandmck? We won't maintain the old one, but you can still reference it up to this point. We will only maintain the latest docs and not backport to the old docs.

crandmck · 2016-12-06T19:44:51Z

whether to add "style-guide-es5.md" to that document? I think that's not necessary, since most back-porting work is done by us, not the community.

Oh, I see, the new document replaces the old one, and you're saying that the community won't generally need the old (ES5) one. OK.

is there a way to "freeze" the old ES5 docs with LB2 ?

AFAIK, the only way to freeze it is to not accept any PRs to change it. We could also add a warning at the top of the page to say that it's specific to 2.x, and is "frozen", etc. Always need to consider that someone can land on the page (without context) from a search.

superkhau · 2016-12-06T19:56:16Z

AFAIK, the only way to freeze it is to not accept any PRs to change it. We could also add a warning at the top of the page to say that it's specific to 2.x, and is "frozen", etc. Always need to consider that someone can land on the page (without context) from a search.

I meant make a second copy of the .md file and add it to loopback.io as historic docs. You can git checkout to the point in time before the updates happened and copy the file directly.

superkhau · 2016-12-06T20:00:07Z

Maybe add a footnote somewhere in the new doc that points to the old docs? Ie. If you want to know the ES5 standard we followed before and not maintaining now, see link-here-to-copied-md-file. @crandmck Thoughts?

crandmck · 2016-12-06T21:28:38Z

make a second copy of the .md file ...

What's the point of that? It's already at http://loopback.io/doc/en/contrib/style-guide-es5.html. The source file will of course live in GH as well.

Maybe add a footnote somewhere in the new doc that points to the old docs?

Already there in the "See also" box at the top-right of http://loopback.io/doc/en/contrib/style-guide.html.

superkhau · 2016-12-07T05:03:36Z

What's the point of that? It's already at http://loopback.io/doc/en/contrib/style-guide-es5.html. The source file will of course live in GH as well.

You're right, there is no point when I think about it more clearly now. The correct approach is probably to link directly to the commit before the new changes.

We keep a reference to the pre-ES6 docs linked from loopback.io as required for users who may want to submit backports (or for us to reference easily)
We also get to keep the latest ES6 conventions in the current loopback.io docs

Thoughts?

bajtos · 2016-12-07T09:27:55Z

@superkhau The current state of things is deliberate. There is a current style guide in style-guide.{md,html} that will keep evolving over time. Then there is a snapshot of the style guide that was in effect at the time LoopBack 2.x was in development. I find it useful to have this old document available on the website, and also have the ability to change it if there ever becomes a need for that. (For example, add a section explaining how to backport changes from ES6 codebase to ES5 codebase.)

We keep a reference to the pre-ES6 docs linked from loopback.io as required for users who may want to submit backports (or for us to reference easily)

We already have that now.

We also get to keep the latest ES6 conventions in the current loopback.io docs

Ditto, we have this too.

I guess I don't understand what are the benefits of your proposal, compared to the current state? Also, is this discussion worth our time?

superkhau · 2016-12-07T18:22:55Z

@bajtos You're right, I just took a closer look at loopback.io. It already looks correct the way it is -- ES6 style guide promoted and ES5 doc in a referenceable link, both updateable if needs be.

I find it useful to have this old document available on the website, and also have the ability to change it if there ever becomes a need for that. (For example, add a section explaining how to backport changes from ES6 codebase to ES5 codebase.)

I was just asking if there was a way to "freeze"/keep a copy of the ES5 docs around so we can have both available and backport if necessary. Since we're already there, this conversation is done.

mnlbox · 2018-06-17T07:37:13Z

Is it available now? I can't find anything with ES6 in Loopback 3 documentation 😞

bajtos · 2018-06-18T14:52:19Z

Is it available now? I can't dind anything with ES6 in Loopback 3 documentation 😞

This pull request is adding/modifying the style guide for people contributing changes to Loopback, it's not primarily aimed at people writing applications using LoopBack as the framework.

The content of this guide is available here: http://loopback.io/doc/en/contrib/style-guide.html

Update style guide for ES6

0fbfe41

- Copy the existing guide to style-guide-es5 - Add few ES6-specific sections - Update all code examples to use the new ES6 syntax

bajtos assigned superkhau Dec 5, 2016

bajtos added the #review label Dec 5, 2016

bajtos mentioned this pull request Dec 5, 2016

Drop support for Node v0.10 and v0.12 strongloop/loopback#2807

Closed

11 tasks

bnoordhuis reviewed Dec 5, 2016

View reviewed changes

fixup! Apply Ben's feedback

0106dea

bajtos force-pushed the update/style-guide branch from b85c1ab to 0106dea Compare December 5, 2016 11:54

bnoordhuis approved these changes Dec 5, 2016

View reviewed changes

crandmck merged commit f322838 into gh-pages Dec 5, 2016

crandmck removed the #review label Dec 5, 2016

crandmck deleted the update/style-guide branch December 5, 2016 17:36

bajtos mentioned this pull request Dec 6, 2016

Honour allowExtendedOperators in "DAO.find" loopbackio/loopback-datasource-juggler#1180

Merged

eddiemonge reviewed Dec 6, 2016

View reviewed changes


		### Classes

		ES6 introduced syntax-sugar for defining classes, start using its' advantages


		### Variable declarations

		Prefer to use immutable variables declared with `const` keyword:


		### Arrow functions

		The are two considerations to keep in mind when deciding whether to use an

Conversation

bajtos commented Dec 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Dec 5, 2016

Uh oh!

crandmck commented Dec 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

superkhau commented Dec 6, 2016

Uh oh!

bajtos commented Dec 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

superkhau commented Dec 6, 2016

Uh oh!

crandmck commented Dec 6, 2016

Uh oh!

superkhau commented Dec 6, 2016

Uh oh!

superkhau commented Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crandmck commented Dec 6, 2016

Uh oh!

superkhau commented Dec 7, 2016

Uh oh!

bajtos commented Dec 7, 2016

Uh oh!

superkhau commented Dec 7, 2016

Uh oh!

mnlbox commented Jun 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bajtos commented Jun 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

crandmck commented Dec 5, 2016 •

edited

Loading

superkhau commented Dec 6, 2016 •

edited

Loading

mnlbox commented Jun 17, 2018 •

edited

Loading