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

Update 'JavaScript coding style' page #936

Merged
merged 6 commits into from
Oct 29, 2024
Merged

Conversation

romaricpascal
Copy link
Member

@romaricpascal romaricpascal commented Aug 22, 2024

Updates the 'JavaScript coding style' page upper sections based on feedback from Frontend Community (either live or through this document (internal link)). Each commit updates a separate section for easier review.

The biggest update is the 'Linting' section, which now tries to paint a clear picture of the available options between 'standard', 'ESLint' and 'Prettier', as well as where to run the linters.

The sections about 'Modules' and 'Strict mode' could use a big overhaul as well, which I'll do in a separate PR to focus the discussion:

  • as we're moving to using ES Modules, 'strict mode' and isolation from other modules is provided by default rather than needing manual addition and IIFEs
  • we probably need to distinguish modules (as in a file that contains code, exporting features and importing other modules) from the classes that provide functionalities and are called modules as well so far
  • with tools like Babel providing transpilation and polyfilling, there's a chance we need to clarify whether we talk to the modules being sent to the browser or the modules being authored by the teams.

@romaricpascal romaricpascal changed the title Update JavaScript code style page Update 'JavaScript coding style' page Aug 22, 2024
@romaricpascal romaricpascal force-pushed the javascript-coding-style branch 3 times, most recently from 8c6bdee to 4150b37 Compare September 13, 2024 14:50
@romaricpascal romaricpascal marked this pull request as ready for review September 13, 2024 14:51
Copy link
Contributor

@andysellick andysellick left a comment

Choose a reason for hiding this comment

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

Looks good. Just a few suggestions/comments.


#### Prettier

Prettier's only preoccupation is with only with [code formatting, not code quality][prettier-comparison].
Copy link
Contributor

Choose a reason for hiding this comment

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

is with only with - should this be is with?

#### Prettier

Prettier's only preoccupation is with only with [code formatting, not code quality][prettier-comparison].
It can be used as a complement to ESLint for further automated formatting, with much more advanced decisions in terms of indentation, spaces, line breaks,...
Copy link
Contributor

Choose a reason for hiding this comment

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

This line ends with ,... is that meant to be there? If it's markdown apologies, not familiar with it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just poor copy ending the sentence with an ellipsis. I'll rephrase :)


Prettier's only preoccupation is with only with [code formatting, not code quality][prettier-comparison].
It can be used as a complement to ESLint for further automated formatting, with much more advanced decisions in terms of indentation, spaces, line breaks,...
It runs as a separate command (`npx prettier`) and the []`eslint-config-prettier`][prettier-linters] ensures there'll be no conflicts between the
Copy link
Contributor

Choose a reason for hiding this comment

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

Should

[]`eslint-config-prettier`][prettier-linters]

be

[][`eslint-config-prettier`][prettier-linters]

?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be:

Suggested change
It runs as a separate command (`npx prettier`) and the []`eslint-config-prettier`][prettier-linters] ensures there'll be no conflicts between the
It runs as a separate command (`npx prettier`) and [`eslint-config-prettier`][prettier-linters] ensures there'll be no conflicts between the


Prettier's only preoccupation is with only with [code formatting, not code quality][prettier-comparison].
It can be used as a complement to ESLint for further automated formatting, with much more advanced decisions in terms of indentation, spaces, line breaks,...
It runs as a separate command (`npx prettier`) and the []`eslint-config-prettier`][prettier-linters] ensures there'll be no conflicts between the
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence seems incomplete? ...ensures there'll be no conflicts between the

#### On CI

Running standard in CI ensures that all pull requests meet our code conventions before getting merged on the `main` branch.
You should at least have this set up on your project.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: I'd maybe reword this as You should have this configured as part of your project.


Waiting for CI to know if the code follows the convention can take a bit of time.
A pre-commit Git hook allows to get quicker feedback, directly on developers' machines.
Errors that are automatically fixable can be fixed at that stage without human intervention, as well,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Errors that are automatically fixable can be fixed at that stage without human intervention, reducing the effort of linting for developers.


The standard docs have a [complete list of rules and some reasoning behind them](https://standardjs.com/rules.html).
To get even quicker feedback, editor plugins can highlight issues while editing files.
They also allow to fix automatically fixable errors on save, saving further fixing effort.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: They can correct automatically fixable errors on save, saving further development effort.


Standard is also [widely used (warning: large file)](https://github.com/feross/standard-packages/blob/master/all.json) (which means community familiarity) and has a [good ecosystem of plugins](https://standardjs.com/awesome.html).
Each of the tools previously listed has plugin to help integrate with editors:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be plugins?

var name = 'thing'
function query () { ... }
```
**Why:** This lets future developers know how to interact with objects and sets the appropriate affordances. It also follows the conventions of the standard library.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: is there an alternative word we could use to affordances? I had to look up what it means, maybe we could use something more commonly used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good shout, I copied that from the previous content without thinking further. I'll rephrase.

As the ecosystem for JavaScript linting has evolved, with ESLint offering the possibility to do extra linting than standard and Prettier providing opinionated formatting, the section tries to present the different alternatives, but keeps emphasising that we shouldn't spend our time picking rules.

It also adds a little more details on when to run linting.
Add mention of editorconfig files to help ensure whitespace is consistent
Adds extra section for easier findability and updates the syntax of examples to ES6
It's no longer as trendy as it used to be, with the JavaScript ecosystem now focusing more on native JavaScript or TypeScript
We're advising against the use of jQuery so our examples should follow suite
@romaricpascal
Copy link
Member Author

@DavidBiddle @andysellick I think I've addressed all your feedback, let me know if there's still something off 😊

Copy link
Contributor

@DavidBiddle DavidBiddle left a comment

Choose a reason for hiding this comment

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

You've fixed everything I raised and I think everything @andysellick raised too.

Looks great now, and a really useful update overall. Thanks for doing this! 🎉

@romaricpascal romaricpascal merged commit 3c519ed into main Oct 29, 2024
1 check passed
@romaricpascal romaricpascal deleted the javascript-coding-style branch October 29, 2024 17:06
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.

3 participants