Skip to content

Modernization of (static) code examples #143

Unanswered
teoli2003 asked this question in Code examples
Modernization of (static) code examples #143
Jun 29, 2022 · 22 answers · 81 replies

Hi all!

Our JS documentation dates back to ES3. We completely revamped it around 2014 (to ES5), and since then, we have kept up with new features in JS.

Keeping up means documenting the new features, but not necessarily updating all the previous docs to use these. Note that we don't want to use the fancy new features immediately everywhere as it takes a few years before they really become mainstream.

Nevertheless, a lot of our examples feel outdated, and beginners reading or using them will become used to outdated habits, which is not our goal. As an example, if you don't use for...of when possible your code doesn't go through many review process; the same if you use only var

We at Open Web Docs have started to update these examples, feature by feature (e.g. using const and let instead of var), with the help of the whole MDN community.

I would like to launch the discussion here on the JS features we should use everywhere it is possible (and how to detect MDN pages that need to be updated). The idea is to create small-to-medium-size projects to fix them little by little.

What are the drawbacks? Given we will only use features that are widely supported, and not jump on the latest addition right away, there are very few negative points: mostly it will make the life of devs needing to support IE a bit more difficult, but with its obsolescence now officially acted, and most of these features already used in every blog and all over the web, it shouldn't bother us much.

I have a few entries to launch the discussion (I will update the list as the discussion progresses).

  1. Use const and let instead of var [ES6/2015]
    This one is in progress: No more vars in JavaScript docs content#16614 and No more vars in Web/API content#16662 thanks to @wbamberg and numerous contributors (👏 )
  2. Use for...of instead of the traditional for(;;) when going through array-like structures.
    We can find pages by looking for for loops calling the length property on the structure.
    In progress, too: Modernize JS in Web/API: Use for...of when possible content#18103 (Help welcome! 🧑‍🤝‍🧑 )
  3. Use arrow functions where possible. [ES6/2015]
    We could look at the function keyword in samples, and check.<
    Note that we want to keep the parentheses around the parameters, even when there is only one.
  4. Use implicit return (a.k.a. concise body) with arrow functions where possible. [ES6/2015]
    (e) => { return e.id } can be written (e) => e.id. (blog)
  5. Use template literals where possible [ES6/2015]
    We can look for 'text' + ... + 'text' in samples to find most (but not all) cases.

Additions from the discussion here:

  1. Use Object.hasOwn() instead of Object.hasOwnProperties() [ES2022]
    Supported by every browser; about 26 pages to update.
  2. == -> === [pre-ES6]
    This is standard nowadays.
  3. Replace forEach with for...of [ES6/2015]
    Use for...of if possible, then forEach(), and finally for(;;) if needed.
  4. Fix function definitions that could use the method definition syntax [ES6/2015]
    Example of such change.
    In progress: Added shorter method definitions in objects for code examples content#18370
  5. Replace a || b and a ? a : b by the nullish coalescing operator (??), and the optional chaining operator (.?) when pertinent. link to a concrete example [both ES2020]
  6. Promise.finally() [ES2018]
  7. Use async functions (await) (they already are ubiquitous on MDN for good reasons…) [ES2017]

Already in our guidelines, but not 100% enforced:

  1. Use shortcuts for boolean tests — use x and !x, not x === true and x === false
    Already required by our guidelines, but we have about 32 pages with such erroneous tests.
  2. Ban Hungarian notation for variable names.
  3. Use literals — not constructors — for creating arrays (const x = ['bli', 'bla', 'blo'] not const x = new Array('bli', 'bla', 'blo'))
  4. For object's members, use the dot notation (objName.objMember) rather than the bracket notation (objName['objMember']) whenever possible.
    ESLint rule to avoid it.

Rejected:

  • Use the logical assignments (&&=, ||= [ES2021]
    Logical comparison in JS is a bit weird (as they don't return boolean values), so let's avoid this.

Proposed:

  • Use classes instead of constructor functions [ES6/2015]
    ⚠️ No consensus for the moment.
  • Use object destructuring and array destructuring, especially as parameters of arrow functions. [ES6/2015]
    ⚠️ No consensus for the moment.
  • Use ??= as their meaning is obvious when used. [ES2021]
    ⚠️ No consensus for the moment. The idiom may be obvious and becoming quickly common.
  • Use the numerical separators where possible: 1000000 = 1_000_000 as this helps legibility, is supported everywhere and easy to understand. [ES2021]
  • Use optional catch binding: instead of catch(unused) {}, write catch {} [ES2019]

Note: Once we agree on a set of modernization, we must update our generic JS writing guidelines.

Principles. As the discussion progressed, we surfaced a few principles that we use to guide our decision. It will be useful to list them explicitly in the guidelines:

  1. We want our code example and snippets to be easy to read, as we have the goal to teach good JS. A lot of modern JS features, naturally work towards this goal, but sometimes, they can make the code more difficult to understand. In these cases, we should avoid them. Non-exhaustive examples:
    • We always put parenthesis around single parameters of arrow functions: (event) => { handle(); } instead of event => { handle(); }
    • We accept /* */when the comment is intra-line and described a not-used parameter: (value, index /*, array */) = { …}. In such a case, the dev may copy the snippet and immediately knows that they can use the 3rd parameter if they have the need, without having a not-used variable. This would be an exception to the rule asking us to use \\ for comments.
  2. JS features we want to use in our code example must be widely available:
    • All evergreen browsers should support them for some time in released browsers
    • Availability of the feature to transpilers so that people developing for older engines can set up a toolchain. (Our engine, mdn/yari, does this.)

There are probably other modern features we should start using. Any proposals?

cc/ @wbamberg, @sideshowbarker, @Elchi3, @Josh-Cena

Replies

22 suggested answers
·
81 replies

Also

  • Constructor functions to classes (this is tricky because constructor functions may be more illustrative of the prototype chain)
  • == -> ===
1 reply
@teoli2003

I added == -> === (it was on my list but I forgot it…)

For constructor functions, do we have a way to detect them, so we can scope the work?

One question that comes to mind is, How much of this might we be able to check with existing linters or static-analysis tools?

I’m not familiar enough with the relevant JavaScript tools to know what the current capabilities are — but as for a prioritizing what things we should try to modernize first, I think it’d make sense to first do the things that we can check for with automated tools

14 replies
@Josh-Cena

Checking embedded code in Markdown is tricky—could be possible with an ESLint plugin, I guess. However, most of the refactors here are covered by existing rules, e.g.

(If we are to add ESLint and we found a way to make it lint embedded code, we could well just enable eslint-config-airbnb.)

But before that, let’s try to get Prettier set up first, as that’s probably less work overall…

@teoli2003

The more automatic, or semi-automatic, tools we can use the better. Even if they don't fix everything, they can detect potential errors and help with maintenance. (In general, I prefer a 70% solution now to a 100% solution in the future, maybe)

In the past, we have cleaned a lot of bad idioms, like the use of eval() and there is clearly more to do there.

@wbamberg

Re: tooling. Yes, we can and should use ESLint for this. But also, getting set up to run ESLint (and Prettier, and markdownlint) is quite a big project with some tricky corners, and delivers no value until it's done.

So IMO it is still worth manually picking off some of these problems, especially var, because it's a pretty low-commitment project in which every page we fix improves the site, and I'm really happy with the progress that volunteers have made. Where ESLint really shines is in preventing people reintroducing the problems, so it's definitely a part of our long-term plan.

Re: ESLint configs. We already run ESLint for interactive examples and should use a common config.

One question is whether we should fix problems "horizontally" (i.e. clean out all the vars, then all the for(;;), etc) or "vertically" (i.e. fix all the issues in each section as we go). For "no more vars" I have obviously gone for horizontal, because this makes the tasks simpler and thus more amenable to a community-driven project.

Yet another issue in here is dealing with code samples that live outside mdn/content (e.g. in mdn/dom-examples) but are copy-pasted into mdn/content pages. This is a big maintenance problem on MDN generally. We should probably have a strategy of fixing up the code repos first, then updating the pages with the fixed-up code samples. IMO we should also soft-deprecate this practice in favour of using live samples when we can, because maintenance is so much easier with live samples (I know it's not always possible to use live samples).

Note that we don't want to use the fancy new features immediately everywhere as it takes a few years before they really become mainstream.

This really depends on "what" kind of feature it is. Modern JavaScript tooling almost always assumes existence of transpilers/polyfills, so using bleeding-edge features is not a big deal—e.g. Object.hasOwn is definitely worth using, same as class fields.

2 replies
@teoli2003

Yes, you are right: the intended goal of the sentence was to prevent having a rule "It is in stage 3 or 4, so let's use it everywhere". I would prefer for us to define case by case.

Let's add Object.hasOwn to the list: despite being in ES2022, it is supported by every modern browser (we have about 26 pages to fix). It is also a new method, so is much easier to learn than a new language structure (like a new loop or destructuring arguments)

(To all: Feel free to argue if you disagree :-) )

@Josh-Cena

Yes, I was mostly thinking about specced features. I just checked again and none of the stage 3 features seem ready/worthy for consumption (or I'm just not particularly excited about any of those), so stage 4 should be a good line.

Just a question (maybe I'm not yet awake). When should we use forEach or for...of (instead of a classical for())?

I think the advantage for...of will work with any iterable (not just Array, NodeList, and HTMLCollection) and goodies like continue or break are available (and more complex) in forEach. Should we impose one over the others, or just leave them to the taste of the author?

Is there a difference in performance?

14 replies
@wbamberg

I'm not much of a user of forEach personally. I would always use for...of over for;; if I could: it's easier to read, has less state to look after, and avoids certain errors (like getting the start and end points wrong).

@teoli2003

Should we get rid of forEach in favour of for...of then (except on the Web APIs pages that speak about forEach of course)?

Edit: As forEach is part of Web APIs, maybe we should add a note in the relevant articles to use for...of instead, when possible.

@Josh-Cena

I don't have strong feelings, but here are a few arguments for forEach:

  • eslint-config-airbnb bans for...of by default, because its polyfill requires regenerator-runtime, which is very heavy.
  • If you need the array index, you would need forEach.
  • forEach chains more naturally if you are doing array method chaining.

Should we also fix function definitions that could use the method definition syntax [ES6/2015]?

0 replies

We don't have many cases of these, but I think we should explicitly ban – and fix the few occurrences – variables named using the Hungarian notation. I used it in C++ (during another life) and with static typing, it was a pain in the neck. Not something for dynamically typed language, IMO.

1 reply
@teoli2003

Replying to myself. This is already forbidden according to our JS Style Guidelines (not explicitly but as we require lowerCamelCase…) So I'm adding it to the list (checking other guidelines, so that we clean all of it!)

I propose to add:
Avoid String.concat(): Use template literals or the concatenation operators (+ or +=) instead.

2 replies
@Josh-Cena

That sounds more like a stylistic improvement than a "modernization" point? If we get a linter set up we have more freedom about what kind of styles to enforce.

@teoli2003

True.

With @Josh-Cena, we found a place where the new Nullish coalescing operator was handy and clearly a new idiom. link to the concrete example

It replaces some occurrences of the two idioms that were previously used:

a || b
a ? a : b

by

a ?? b

|| always seemed a hack to me in JavaScript, though I use && all the time in the shell 🤷

A usual case is combined with the optional chaining operator:

a.?m ?? b

It is a much newer feature though: ES2020, but well-supported:
Capture d’écran 2022-07-02 à 10 33 25

The optional chaining operator is also ES2020 and well-supported:
Capture d’écran 2022-07-02 à 10 45 57

0 replies

I don't think there is much work to be done, and I think it is already in use on MDN, but I think we should list (to explicitly allow it):

  • Promise.finally() [ES2018]
0 replies

@hamishwillee, I noticed you used object destructuring and I think we should add it to the list of modernization we would like to perform (even if it is less common):

Your example, combined with an implicit return in an arrow function.

const inventory = [
  {name: 'apples', quantity: 2},
  {name: 'bananas', quantity: 0},
  {name: 'fish', quantity: 1},
  {name: 'cherries', quantity: 5}
];
const result = inventory.findLast( ({ quantity }) => quantity < 2 );
7 replies
@Josh-Cena

Destructuring in parameter position is not universal, but destructuring assignment is widely-adopted. See https://eslint.org/docs/latest/rules/prefer-destructuring

@hamishwillee

First time I saw destructuring assignment I had to ask WillB because I couldn't find any information about the syntax I was seeing. So I'm not certain it should be rolled out "everywhere" in the same way as other modernisation, but if it is used, I think the doc to object/array destructuring should be linked.

@Josh-Cena

FWIW—it may seem magical for new learners, but in production it has almost become the "industry standard". All the codebases I've worked on have enabled that rule (also part of Airbnb's ESLint config) and use destructuring assignment by default.

I would like to suggest the logical assignments (&&=, ||= and ??=) as their meaning is obvious when used. [ES2021]

Support is great:
Capture d’écran 2022-07-06 à 12 53 47
Capture d’écran 2022-07-06 à 12 54 38
Capture d’écran 2022-07-06 à 12 55 59

5 replies
@JeremiePat

Not a big fan of enforcing that syntax as it is less obvious than it looks. It is useful and it makes code more compact, but from experience many developers, for whom JavaScript is not their primary language, are easily confused by it (mostly because they are already confused by JS boolean operators as those operators do not return a boolean value). So I don't think we should be hard on it (but I would not discourage it either)

@Josh-Cena

??= is very idiomatic. e.g. obj.someKey ??= []. I don't have strong opinions about others.

@teoli2003

I forgot about the weirdness of logical comparison in JS… So yes, let's wait (at the minimum) for them.

For ??=, I'm unsure now.

I also would like to use the numerical separators where possible: 1000000 = 1_000_000 as this helps legibility, is supported everywhere and easy to understand. [ES2021]
Capture d’écran 2022-07-06 à 13 06 34

4 replies
@pmario

As a user I do like "numerical separators", because they are easier to read. ... But

As a user that searches for reference docs about an eg: ES2018 syntax explanation, I wouldn't expect it. Let's say for whatever reason I'm locked to ES2018 syntax, this little "helper" would force me to switch to ES2021

As a user in general I would expect example code to be within 1 spec only. So in my case ES2018 only

just some thoughts.

@Josh-Cena

@pmario Modern web dev almost always assumes transpilers/polyfills (especially if you have commitments to old runtimes), so we care much much more about approachability of examples than the actual spec version. Also I wouldn't expect, say, the let documentation to not contain anything higher than ES2015 simply because let itself is ES2015.

@teoli2003

I think @pmario raised a very interesting point: many web devs have to target browsers that don't support modern features, and ofter very old browsers.

As pointed out, we cannot target a specific ES version inside a page: this is not practically doable (without even speaking about features that are modified/extended in newer ES versions).

The standard way of solving them is: Web developers write modern JavaScript, then transpile them.

In order to solve the problem raised by Mario, we must be sure that the modern features we want to use in our code samples are available in transpilers, in addition to their availability in engines.

I'm adding this to our Principles list in the original post.

What about optional catch binding: instead of catch(unused) {} we can write catch {} [ES2019]
Capture d’écran 2022-07-06 à 13 11 07

0 replies

Yes, the three syntaxes above are definitely worth using.

1 reply
@teoli2003

As responses can move up and down in GitHub discussions, I'm just listing the three syntaxes here, so that the context of this comment is not lost:

  • optional catch binding
  • numerical separators
  • logical assignment operators

Strangely, I think we have never stated this...

  • Use // for comments. /*...*/ should be available for debugging.
    Note that the pattern fctName(param1, param2 /* unused */, param3) should be worked around (with something similar to fctName(param1, ...[ , param3]) – I need to test if it works).

A linter will find and mark them, but I think we should explicitly state it.

(This discussion will lead to some meta-docs describing what we discuss here and some actions to fix the actual content.

3 replies
@JeremiePat

I prefer clarity over modernity. In this case, for documentation such as MDN, using the /*...*/ comment makes better sens to clarify code sample intent.

@teoli2003

This makes sense to me, you are right. I think we should explicitly allow /*...*/ to show dropped/not used params (and similar).

@teoli2003

To capture this, I started a section about principles in the orignal post.

Principles. As the discussion progressed, we surfaced a few principles that we use to guide our decision. It will be useful to list them explicitly in the guidelines:

  • We want our code example and snippets to be easy to read, as we have the goal to teach good JS. A lot of modern JS features, naturally work towards this goal, but sometimes, they can make the code more difficult to understand. In these cases, we should avoid them. Non-exhaustive examples: …

Not sure if this came up: Lebab is the opposite of Babel and might help with some of the mechanical parts.

1 reply
@teoli2003

I didn't know about this tool and, indeed, this may help for some mechanical parts, but also for some extra points to consider for the modernization (consider = discuss before deciding to enforce or not).

Thanks!

has anybody verified the claim that you will not get the job nowadays if you use var in your interview? I would personally not judge anybody harshly for that.

I have no objection to using modern syntax in the examples but I would (personally) want that claim to be backed up a bit before I did a ton of work to manually convert and test the current correctly-written examples using still-perfectly-valid javascript.

It seems more sensible to use modern syntax when adding or updating examples but accepting the old examples as they are. Updating pages for ancient apis to reflect modern coding styles does no good for the job applicants - would you also not get the job if you use getElementByClassName or getElementByTagName instead of querySelector?

8 replies
@pmario

I think you wouldn't get a job, it could not explain what's the difference between var, let and const. IMO that would be a reason. An explanation should be found at MDN pages, for exactly that reason.

@Josh-Cena

getElementByClassName has its practical usage. var only creates confusion. We'd rather say "use let and the language gets you covered" than say "use var and be careful of hoisting, no block scoping, implicitly creating global properties". While it won't be a determining factor that makes you lose a job, I have yet to see a well-maintained codebase in 2022 using var. It's important for MDN to reflect what code people are writing today.

(In fact, because var is virtually never used today—there's not a single reason why you should use it—many new tutorials are starting to pretend it doesn't exist. That's a good trend.)

@macgyver

so you are expected to understand some parts of the language (the difference between var, let, and const) but not other parts (hoisting, scoping, etc..)?

To be honest most of the things listed in the original post are unfounded, especially this statement:

As an example, if you don't use for...of when possible during a job interview, you don't get the job nowadays;

For example, arrow functions and "traditional" functions are two separate things with different behavior, or for..of and for (;;) are not mutually exclusive, or "use implicit returns with arrow functions wherever possible" is, again, more of a matter of preference than a "rule", some devs like the explicit return.

This whole post seems to be more like a style guide than anything, it's not like some of the old syntaxes are deprecated (or will be in the near future) and can't be used anymore, if that would be the case I could see a reason for this post.

2 replies
@Josh-Cena

Correct—most of them are stylistic. We are mostly striving to make our code examples mirror that written by actual companies today (especially the good ones).

@teoli2003 "Won't get the job" is certainly an overstatement (although it could be a sign of one not keeping up with the trend, which is dangerous in the ever-changing web world). I didn't realize it's there in the post!

@teoli2003

I've rephrased it to speak about the review processes...

And yes, two syntaxes often have different nuances. That's why it isn't a find and replace project, but an improvement of the examples to use the most appropriate syntax.

Use arrow functions where possible.

This doesn't sound like a good idea for beginner's documentation.

3 replies
@Josh-Cena

Most code examples are in the "references" section, where we would at least assume intermediate JS knowledge—or else it would be hard to present code at all. (Also, arrow functions are incredibly widely used today and I've rarely seen function expressions at all—not even sure if more people know function expressions exist than those who know arrow functions!)

Don't worry, we would still write the "guides" section the same way as before—we won't jump to using arrow functions before they are even introduced.

@teoli2003

Yes, the Learning area will not have the same set of rules.

@Vallek

I misunderstood then, thanks for answer!

I think we should update the pattern for...in combined .hasOwnProperty()/hasOwn:

For example:

for (prop in elementStyle) {
  if (elementStyle.hasOwnProperty(prop)) {
     out += "  " + prop + " = '" + elementStyle[prop] + "' > '" + computedStyle[prop] + "'\n";
  }
}

Would become the following, which is much easier to read and understand, even for a beginner:

for (const prop of elementStyle) {
      out += `  ${prop} = '${elementStyle[prop]}' > '${computedStyle[prop]}'\n`;
}

I think a significant amount of usage for .hasOwnProperty() on MDN is this pattern.

(This pattern has been uncovered by @maurer2 in this PR)

3 replies
@Josh-Cena

for...of and for...in are two things: for...of iterates, not enumerates the properties. The equivalent is ... of elementStyle.keys().

@teoli2003

Oh yes, you are right! So:

for (const prop of elementStyle.keys()) {
      out += `  ${prop} = '${elementStyle[prop]}' > '${computedStyle[prop]}'\n`;
}

or even

for (const [index, prop] of elementStyle.entries()) {
      out += `  ${index} = '${prop}' > '${computedStyle[index]}'\n`;
}

And of course, in cases where the index is need only to access the value we shouldn't use keys() at all.

@Josh-Cena

Ah, sorry, I messed up still: should be Object.keys(elementStyle)... If you can use elementStyle.keys() then elementStyle is already an array (or at least not a plain object).

I'd like to vote for linting to require argument parentheses, sometimes having them and sometimes not is an unnecessary confusion that serves no purpose. They should be there all the time for consistency and let the build processes deal with them as necessary.

3 replies
@teoli2003

Do you mean for arrow functions? If so, I agree.

@Josh-Cena

Yes, my personal style (and Prettier's default) would also add parentheses for arrow functions.

@teoli2003

As a note, I'm enforcing it already in reviews. I hope we have Prettier hooked soon so that we can consistently enforce this.

Sometimes saving 2 characters is not worth the hassle. (And there are tools like minify to win it back further down the toolchain).

Edit: I added a note in the original post to make it clear.

The ESLlint enforces the use of double quotes " by default. We should assert in our guide lines list what we want to use for HTML, CSS and JavaScript code.
This will reduce back and forth between contributors and reviewers. And code will look consistent across the website.

7 replies
@Josh-Cena

Not just ESLint—prettier uses double quotes as well (although I think they are seeking to change this in v3). This is more stylistic than modernization so I suggest starting a new discussion—I anticipate some bikeshedding about this.

Also I don't feel like it's really worth enforcing in the short term. I won't reject merging PRs just because they use single quotes while I prefer double.

@teoli2003

Note that the idea to use Prettier is to avoid the bikeshedding about this.

I accept both for the moment, when Prettier/ESLint will be ready to be automatically used, this will be fixed at that point. No need to hunt these.

@Josh-Cena

Both ESLint and Prettier have an option for this, so even if we adopted a formatter we have to bikeshed about how it should be configured 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment