Add a linter for JS examples

[wbamberg]

As Mark suggested, having a linter for the JS examples especially would really help guarantee a consistent style.

[schalkneethling]

@wbamberg I am guessing here you are referring to linting the JS that is contributed i.e. everything inside the code block ~ https://github.com/mdn/interactive-examples/blob/master/live-examples/js-examples/array-concat.html#L2

[wbamberg]

@schalkneethling , yes, exactly.

[7ayushgupta]

I'd like to try working on this issue. Can you please guide me on how to proceed?

[wbamberg]

That would be great! @schalkneethling can give you better guidance than me on the best way to do it.

[7ayushgupta]

@schalkneethling, can you please help me on how to begin and what all to do?

[schalkneethling]

@7ayushgupta I will get back to you as soon as possible with some ideas. Basically I was first thinking to make it part of the main page builder Nodejs script here[1] but, after thinking about it a bit more, I am thinking a standalone script utility would be a better way forward.

These two can then be run independently or together, which offers us more flexibility. Do you have experience with writing Nodejs utilities that does file parsing etc?

[1] https://github.com/mdn/interactive-examples/blob/master/index.js

[7ayushgupta]

No, I don't have experience as such, but I will surely learn things that are required. If the issue is not urgent, I can work on it, taking my time.

[schalkneethling]

@7ayushgupta This one is pretty high on our list. I am wondering, perhaps one of the items on this list might be of interest to you? https://github.com/mdn/interactive-examples/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc+label%3Agood-first-bug

[7ayushgupta]

Would you be using something like JSHint or write the linting code yourself? I guess the latter to avoid any dependencies which may occur.

[schalkneethling]

I will be using the eslint Nodejs API

[wbamberg]

See also https://github.com/mdn/interactive-examples/pull/1470.

[ikarasz]

Hi @wbamberg, @schalkneethling, I would be interested in this improvement but I am not familiar with the ecosystem... Is this related to mdn-bob? Could you pls give me some hint? Can this fit into the processAndWrite function?

Also, I was thinking about the examples, they are html files now, could we change them to js files? The build process could wrap them into the pre and code tag, so we would have syntax highlight and proper file extensions in the interactive-examples repository. The linting would be much easier if they would be simple js files, am I wrong?

[wbamberg]

The linting would be much easier if they would be simple js files, am I wrong?

You are not wrong! I agree this would the the first step towards setting up linting. Making them js files would be easier for contributors too I think. And yes, to make this happen we'd need changes to mdn/bob to handle this style, and to mdn/interactive-examples to update all the JS examples.

In fact I've been a little way down this exact path quite recently. What I did was:

• added a new export to lib/processor.js, that looks something like:

/**
 * Process the example source code, based on its type.
 * @param {String} type - `html`, `js`, or `css`
 * @param {String} source - The source code itself
 */
function processExampleCode(type, source) {
    switch (type) {
        case 'html':
            return processor.preprocessHTML(source);
        case 'css':
            return source;
        case 'js':
            return `<pre><code>${source}</code></pre>`;
    }
}

• inside build() in pageBuilder.js, replace this: https://github.com/mdn/bob/blob/master/lib/pageBuilder.js#L71-L83 with something like:

let exampleCode = fse.readFileSync(currentPage.exampleCode, 'utf8');
exampleCode = processor.processExampleCode(
    currentPage.type,
    exampleCode
);
outputHTML = tmpl.replace('%example-code%', () => exampleCode);

But I didn't get as far as actually testing that it works :(

[schalkneethling]

Agreed. Everything build related would need to happen in BoB and then this repo will need to be updated to reflect the changes to files names, structure etc.

[ikarasz]

I'm working on this issue now and I was thinking about how do we want to replace the current examples? All of them at once or in smaller chunks in different pull requests?

[schalkneethling]

I'm working on this issue now and I was thinking about how do we want to replace the current examples? All of them at once or in smaller chunks in different pull requests?

I would think that, if we do a couple of small pull requests first, that would be the safer option. Once we are confident that the approach works well, we can then do a mass migration. Thoughts @wbamberg

[wbamberg]

I'm working on this issue now

\o/

@schalkneethling is probably right, it would be safer to do it that way. So I suppose it would go:

• [x] update mdn/bob so it can understand both formats (HTML containing JS or pure JS)
• [x] update a few examples to be pure JS
• [x] check that everything still works
• [x] update all the examples to be pure JS
• [x] update mdn/bob so it only understand pure JS

After that we would be in a position to lint the example sources.

Does that make sense? If so, shall we file separate issues for these steps?

[schalkneethling]

Sounds like a plan to me.

[ikarasz]

Sounds perfect to me too

[wbamberg]

I have filed https://github.com/mdn/bob/issues/367 over in mdn/bob for the first of these.

[ikarasz]

@wbamberg how can I get notified when the mdn/bob has released?

[schalkneethling]

@wbamberg how can I get notified when the mdn/bob has released?

I will post a message here once it has been released.

[schalkneethling]

@ikarasz - A new version of the package mdn-bob (1.1.42) was published at 2020-01-14T14:04:00.544Z from 35.202.245.105. The shasum of this package was 0e77590977d33ea1938e85fd90f55f7750f7c246.

[schalkneethling]

https://github.com/mdn/interactive-examples/pull/1525

[ikarasz]

Cool @schalkneethling, then as next step we need to add some examples with js source. I'll do that tomorrow.

[ikarasz]

It looks good to me. Can we start to replace all the examples? I would go with smaller chunks, for example by folders.

[wbamberg]

I believe the three examples you updated are these, and are now live:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/extends https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/getUTCSeconds https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/now

...and they look great. So I'm totally happy to go ahead with updating the examples!

[wbamberg]

I merged https://github.com/mdn/interactive-examples/pull/1564 yesterday, which means we are no longer supporting JS-in-HTML, and apparently nothing has exploded. So the list in https://github.com/mdn/interactive-examples/issues/250#issuecomment-565534930 is now complete. Great work on this @ikarasz and @schalkneethling !

[ikarasz]

Good news @wbamberg! What's next? In my opinion, the linter should be invoked by mdn/bob during the build, but we should also let contributors check their change, eg with the npm start command. On the other hand, the eslint config should be in this repository, so we can change it anytime without deploying a new bob version moreover bob would be more flexible in this case. What do you think?

[wbamberg]

Do you think we should have a separate npm lint command, and call it from npm build? The main things functionally seem to be:

• it should be convenient to run it locally from the command line
• it should run as part of CI
• as you said, it should be convenient to change the rules, which means having them in interactive-examples

Apart from that I'm happy to defer to you and @schalkneethling about how/where we should integrate linting.

There's also a big question about which rules exactly to include, but I guess we can postpone that until we have something working.

[schalkneethling]

I do not think Bob needs to be concerned with that. The code is authored in this repo so, this repo should take care of the linting.

I am thinking we use eslint rules that are shared among other projects we have(probably use Kuma as an example?). We can then potentially setup eslint to:

1. Be configurable for your editor (only lint JS files in the relevant directories)
2. Add a pre-commit hook to also lint
3. Add the lint npm script to the list of targets that are run during CI

[ikarasz]

Sounds good to me @schalkneethling, only one question. Does your last point refer to travis?

[schalkneethling]

Sounds good to me @schalkneethling, only one question. Does your last point refer to travis?

CI === Travis === Yes 😄

[ikarasz]

Hi @wbamberg,

I've played around with eslint in this repository and some ideas, questions came into my mind. First, we already have an eslint setup, it is pretty similar to the kuma eslint but it has some unnecessary settings/globals. Overall it looks good, it just needs some adjustments.

If I run eslint with the current configuration, I get ~720 errors. If we change the indentation to 2 spaces, the error count will be reduced to around 270, after autofix only 120 remains. That is pretty good I think.

I was able to fix some of them manually, like:

• ES6< features like optional chaining, generators -> babel-eslint is needed
• double quotation marks where the text contains a single quotation mark -> I've escaped the quotation mark within the text
• no-prototype-builtins -> I've just simply turned off the rule

Unfortunately, despite all my effort, there are still 51 errors and we need to make some decisions. The remaining errors can be divided into 3 distinct groups:

• unused parameters/variables
  • We always indicate the parameters for built-in methods, even if we don't use them, which is good in our case, but eslint doesn't like it. I would say we can turn it off, but in this case, we need to pay attention to them during the code review.
  • An unused variable can be found in Symbol constructor example. I see why we have that, but still, I would rather change the code instead of turning off the rule.
• demonstration of errors or language constructs which are discouraged to use
  • empty blocks
  • catching ReferenceErrors
  • no-redeclare
• capitalization issues for constructor functions
  • I don't know if these were intentional, but here, again, I would change the code.

After this investigation, I think we need some custom solutions. This repository has to contain some bad practices too, not only the best practices because it is intended to demonstrate the elements of JavaScript. We need to disable some eslint rules in some files, but enforce them in other files. I can see the following options:

• permissive config (only styling, indentation, equal signs, etc.)
  • easy to contribute/maintain/setup
  • errors/issues can stay undiscovered and we don't really make the code review easier
• file-based eslint overrides
  • won't pollute the src code, solves all the issues above
  • harder to contribute, because eslint config needs to be updated sometimes (also it will get ugly after a while)
• turn off rules per file with comments
  • easy to handle for contributors
  • bob must be updated to remove these comments

Whatever direction we choose, I would go with smaller pull requests, and setup eslint in smaller steps. The following steps look reasonable to me:

• setup basic eslint
  • check only indentation, quotation marks, and other styling rules
  • add commit hook and npm script
• incrementally tune the config and fix the remaining issues in smaller chunks

[wbamberg]

@ikarasz , thanks for this great analysis! Very interesting. I'll have a proper think about it tomorrow. I've also asked in the team if anyone else has views about how to resolve these issues, so there might be some other responses here too :).

[chrisdavidmills]

This is really interesting. Is there a way to make eslint skip over particular blocks of code that are marked as "bad", in the same way we currently have code examples marked as "bad" in the current KS world?

e.g.

<pre class="example-bad">
  ...
</pre>

An equivalent in markdown could then be

```js-bad
  ...
```

Is this useful, or am I barking up the wrong tree?

From the options @ikarasz has outlined, "turn off rules per file with comments" seems to be the most manageable to me.

[schalkneethling]

This is really interesting. Is there a way to make eslint skip over particular blocks of code that are marked as "bad", in the same way we currently have code examples marked as "bad" in the current KS world?

You can totally do that:

https://eslint.org/docs/user-guide/configuring#disabling-rules-with-inline-comments

[wbamberg]

This is really interesting. Is there a way to make eslint skip over particular blocks of code that are marked as "bad", in the same way we currently have code examples marked as "bad" in the current KS world?

You can totally do that:

https://eslint.org/docs/user-guide/configuring#disabling-rules-with-inline-comments

I this think is @ikarasz 's option 3: "turn off rules per file with comments", isn't it?

As he says though, we don't really want eslint comments appearing in the actual examples, so we'd need machinery in bob to remove them.

Of the three options you've described:

• permissive config (only styling, indentation, equal signs, etc.)
  • easy to contribute/maintain/setup
  • errors/issues can stay undiscovered and we don't really make the code review easier
• file-based eslint overrides
  • won't pollute the src code, solves all the issues above
  • harder to contribute, because eslint config needs to be updated sometimes (also it will get ugly after a while)
• turn off rules per file with comments
  • easy to handle for contributors
  • bob must be updated to remove these comments

I like option 2 the best. Option 1 doesn't provide enough help, as you say. Option 3 I think is a bit too magical: it's very nice for the source shown in the editor to be exactly the same as the source as it is written.

Option 2 seems like the most declarative option. I think a little friction is OK and even desirable, as it makes us think about whether we should really relax the rule in this case. If there are lots of overrides, I agree it would get ugly, but we should keep in mind that this is quite a mature project - we already have a good idea of how ugly it would be, and it's unlikely to change very much in the near future (if we ever support interactive examples for Web APIs (which I would love, but it's years out as best) then we might want to reconsider, but even then I think this will be OK).

there are still 51 errors

I stuck the list in a spreadsheet and made some recommendations about whether to fix the code or override the rule, although I'm very open to argument on them: https://docs.google.com/spreadsheets/d/1kOyfdZXMaamC-peFuLd2dXFXxP0TzTBiIX2eUlxuNP0/edit?usp=sharing. I've ended up with 39 overrides, which doesn't seem excessive. I think I've been pretty conservative (meaning, I've tended to choose "override" where I could have chosen to fix the example), and would be happy to be less conservative it people think that's a good way to go.

The remaining errors can be divided into 3 distinct groups:

• unused parameters/variables
  • We always indicate the parameters for built-in methods, even if we don't use them, which is good in our case, but eslint doesn't like it. I would say we can turn it off, but in this case, we need to pay attention to them during the code review.

Yes, I count 37 instances of "no-unused-vars". It's tempting to override this one globally, that would leave us with only a handful to override per-file. On balance though I think we should not override it globally.

• An unused variable can be found in Symbol constructor example. I see why we have that, but still, I would rather change the code instead of turning off the rule.

I marked this one "override" but I'd be happy for us to fix the example instead.

The following steps look reasonable to me:

setup basic eslint

• check only indentation, quotation marks, and other styling rules
  • add commit hook and npm script
• incrementally tune the config and fix the remaining issues in smaller chunks

Yes, I think this seems like an good plan. Alternatively we could set it up with the config we really want, then incrementally fix the errors? This would motivate us more to get them finished. But either way is fine.

Also, if you want, we could share the work of updating the code to fix errors, but we'd have to coordinate the work then.

[ikarasz]

Exactly, option 3 was what @chrisdavidmills and @schalkneethling meant. based on your feedback I will setup our eslint config with all the desired rules but they will be only warnings until we fix all the issues, after that, we can change them to errors. @wbamberg you convinced me with the overriding approach, it sounds reasonable. As a next step, I will create a pull request which will contain

• fully configured eslint
• new npm script
• fix for trivial errors or simple rule overrides

Then we will figure out what to do with the remaining questionable issues and add the check to travis.

About the commit hook, client-side hooks are not copied during clone or pull so I have no idea how to run the linter before commits. @schalkneethling do you have any experience with this?

[wbamberg]

I think we can close this now? Thanks for all your work on this @ikarasz ! This is the most exciting this to happen to interactive examples for years. If you would like to, please send me an email at my username here at mozilla dot com.