Linting overhaul

[SleeplessByte]

REVIEWERS: you only need to review this message (the PR description). The PR itself is the implementation of this description + syncing files. If you really want to review it, you only need to look at 89997af (#1094)

---

This PR aims to further implement #44, improve on #645, and sorta fixes #681.

• Use a shared configuration for all the student editable files: @exercism/eslint-config-javascript
• Use a shared configuration for all read-only and maintainer only files: @exercism/eslint-config-javascript/maintainers
• Fix a bug where files inside .meta were not (always) linted
• Run the stricter configuration when doing scripts/lint

Student rules

Furthermore, it enables the following rules (on top of the current recommended set) that often resolve actual errors (and are not really style dependent) for students:

Warnings

• array-callback-return
• eqeqeq
• guard-for-in
• no-extend-native
• no-implicit-coercion
• no-promise-executor-return
• no-shadow
• no-unreachable-loop
• require-atomic-updates

These will not block the student (ever), but almost always indicate mistakes. If you'd like to discuss why these were chosen, let me know, and I can give ample examples of actual student code as well as documented examples that these rules solve. Note that none of these rules are listed under "style" rules. In an upcoming update to the analyzer, these warnings will be shown to the user as actionable.

Errors

• default-param-last
• no-eval
• no-unsafe-optional-chaining

These are always mistakes, hence they are errors. In an upcoming update to the analyzer, these warnings will be shown to the user as essential and effectively soft-block the student.

Maintainers / Contributors

Same list as for students but everything is an error. On top of that, the following rules are added to guard for mistakes with import and export statements, which are vital in the stubs and the test files.

In a previous track-wide update we've chosen for named exports (with one or more exceptions) which is why we discourage default exports. Additionally, the extension inclusion/exclusion is for consistency. The unresolved imports ensures all stubs export all the required bindings.

Warnings

• import/no-default-export

Errors

• import/extensions with { js: 'never', json: 'always' }
• import/no-unresolved

Fixes #681

[SleeplessByte]

(I intend to merge this Friday the latest but feel free to voice your concerns, ideas, and/or anything else)

[neenjaw]

@ErikSchierboom @iHiD

[junedev]

Great work! Thanks for taking the time to work through this.

(Possibly) Stupid question: Where is the source for @exercism/eslint-config-javascript? On npm the readme is empty and it does not show a repository.

I mostly like the rules you added.
I would not have picked guard-for-in though. Imo if someone just created a simple object literal and now wants to iterate over it, for-in without guard is just fine. This object literal scenario is also what is most likely to happen when a student uses for-in in an exercise. Forcing the guard although it is not necessary kind of defeats the point of for-in as a readable, short iteration helper. Imo for-in is demonized too much. Just like many things in JavaScript, it has its pitfalls/limitations.

The second thing I am not super happy about is the new formatting for the switch statements (not sure which rule is responsible for the change here). Tbh I never understood the advantage and I thought this "no indentation for the cases" style went out of fashion a while ago. Two reasons why I am not a fan. 1. In all other occurances we always indent the code inside a code block so it makes sense to be consistent. 2. Most docs, tutorials etc show the indented version. When a student comes from a language where indentations matters, the different versions might be confusing.

Feel free to ignore my comments and merge anyway. Just giving my 2 cents.

[SleeplessByte]

(Possibly) Stupid question: Where is the source for @exercism/eslint-config-javascript? On npm the readme is empty and it does not show a repository.

Waiting on Jeremy to create it for me.

• https://npm.runkit.com/@exercism/eslint-config-javascript/index.js?t=1618402770885
• https://npm.runkit.com/@exercism/eslint-config-javascript/maintainers.js?t=1618402770885

I would not have picked guard-for-in though. Imo if someone just created a simple object literal and now wants to iterate over it, for-in without guard is just fine. This object literal scenario is also what is most likely to happen when a student uses for-in in an exercise. Forcing the guard although it is not necessary kind of defeats the point of for-in as a readable, short iteration helper. Imo for-in is demonized too much. Just like many things in JavaScript, it has its pitfalls/limitations.

@june Yeah. I struggled with this one. So I've seen mistakes in student code because they didn't know, but there are also so many cases where it's fine. I guess what we can do here is remove this one from the config, but use the analyzer to show an informative pitfall if someone uses for in. Do you like that solution?

The second thing I am not super happy about is the new formatting for the switch statements (not sure which rule is responsible for the change here). Tbh I never understood the advantage and I thought this "no indentation for the cases" style went out of fashion a while ago. Two reasons why I am not a fan. 1. In all other occurances we always indent the code inside a code block so it makes sense to be consistent. 2. Most docs, tutorials etc show the indented version. When a student comes from a language where indentations matters, the different versions might be confusing.

Nah, that's prettier, I think. I rather have it indent too. Can you point me to a line/file in this PR where this happens? I will try to find the tool responsible and "configure" it correctly, because I am 💯 % with you.

[junedev]

I guess what we can do here is remove this one from the config, but use the analyzer to show an informative pitfall if someone uses for in.

Sounds like a good solution to me.

[SleeplessByte]

Weird. This prettier.io playground link seems correct to me. Is that not what you're expecting @junedev?

[junedev]

here is an example for the changed indentation

[SleeplessByte]

Thanks! Perfect. I will make sure this is right after we merge @junedev

[junedev]

I looked at that commit that contains only the actual code changes that you posted. Maybe it was not reflecting the latest version. Playground example is correct.

[junedev]

I checked the latest version in the branch. Switch statement is correctly formatted there. Sorry for the confusion. 🙈

[SleeplessByte]

I checked the latest version in the branch. Switch statement is correctly formatted there. Sorry for the confusion. 🙈

I think you were right. So something removed the indentation and then formatting added it back 🤷🏽. This was likely solved by adding the prettier-disabled rules to the maintainers config (which removes eslint rules that prettier solves).

[SleeplessByte]

Solved with exercism/eslint-config-javascript#1