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

Remove old-fashioned semicolons #2

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove old-fashioned semicolons #2

wants to merge 1 commit into from

Conversation

edgardz
Copy link

@edgardz edgardz commented Jan 22, 2024

This is always a controversial topic.

But yet again, here we are... 😅

Given the number of linters and formatters we have set up, we can safely get rid of semicolons.
A while ago, I used to be heavily opinionated about enforcing them everywhere until I worked on a modern codebase where their absence made absolutely no difference, and on top of that, the code somehow looked super-clean, modern and friendly (semis were not the only responsible for that feeling, but they definitely were part of it). I was pretty surprised by that, and since then, I removed them from every project I was in charge of without any issue.

(again, prettier will add them automatically when required).

@ThaNarie
Copy link
Member

This is always a controversial topic

I agree with that!

Remove old-fashioned semicolons
worked on a modern codebase
the code somehow looked super-clean, modern and friendly

It's funny you mention this old vs modern, following the hype (although coffee-script also tried it years ago) :D


Some background

Automatic Semicolon Insertion is a mechanism that the JavaScript engine uses to automatically insert semicolons in your code where it believes they are missing. This can lead to code that appears to work but may have subtle bugs.

A good explanation can be found on MDN, along with a bunch of use cases where things can go wrong.

Some entertainment can be found in an old discussion on Github between Bootstrap and JSmin maintainer Douglas Crockford, and a response from Brendan Eich.

Arguments for omitting semi's can be read here and here. They mostly highlight that experienced/real JS programmers should know the exact syntax rules so they know when to and not to use semi's, instead of just adding them everywhere to hope not run into ASI issues.

Arguments

So, most discussions about semi-colons are about two arguments:

  • not adding semicolons can result in bugs (but you should know when that's the case, and do it correctly)
  • people preferring them vs not, clean code, they are optional, etc

Prettier / standard

For the first argument, people mention modern tooling like prettier, or minifiers, that can automatically add them where needed.

While this is true, it's actually more nuanced. It's best to read through prettier's and standard's documentation about it.

Both tools add semicolons at the beginning of "dangerous lines" (lines beginning with `, ( or [), to prevent lines added before them to merge into them:

if (shouldAddLines) {
  ;[-1, 1].forEach(delta => addLine(delta * 20))
}

;(function () {
  window.alert('ok')
}())

;[1, 2, 3].forEach(bar)

;`hello`.indexOf('o')

But more importantly:

Note that if your program currently has a semicolon-related bug in it, Prettier will not auto-fix the bug for you. Remember, Prettier only reformats code, it does not change the behavior of the code.

console.log('Running a background task')
(async () => {
  await doBackgroundWork()
})()

// If you feed this into Prettier, it will not alter the behavior of this code.
// instead, it will reformat it in a way that shows how this code will actually behave when ran.

console.log("Running a background task")(async () => {
  await doBackgroundWork();
})();

or

a = b + c 
(d + e).print() 

// turns into

a = b + c(d + e).print()

Standard says (which is reasonable, but still):

Never start a line with (, [, `, or a handful of other unlikely possibilities.

So this means that you

  1. have to be mindful of what your code looks like after being formatted, as the re-formatted code might indicate you forgot a semi in a specific scenario. Still useful!
  2. understand to recognise dangerous patterns that prettier will not fix, and add a semi there yourself!

And then there are still cases that prettier leaves alone, but look different from what they actually do, like:

function getCheese() {
  return
  {
    cheeseType: "Gouda"
  }
}

Which seems to return an object, but actually returns undefined, and defines an empty object after it.

Luckily we have other tools that spot "dead code", so you would be notified about this one way or another.

How code feels

I believe there are two main camps;

  1. the ones that like to de-clutter, and semi's are just in the way
  2. the ones that like that semi's clearly indicate the end of a statement, and looking for them when scanning code

Personally I fall in the 2nd camp, especially when chaining code where newlines are missing (note, they aren't added by prettier in these cases):

items
  .map((x) => callSomethingWith(x))
  .filter((x) => isXSomethingWeNeed(x))
  .map((x) => doSomethingExpensive(x))
andHereWeCallAFunction(items)
andThis.is.another.line = someValue

Even though there is indentation to indicate what part belongs to the chaining bit, when I'm scanning the right side of the code, I have no way of seeing which line is the last one of the statement.


I also found an interesting comment:

I'll give a different reason for why you might wish to keep them around. There are the actual JavaScript-specific reasons to use it, but it's also good to use as a parser recovery token. At least, for your IDE's parser that is ran every time you type.

Semicolons act as an error boundary in the context of parsing. If you're typing and have some incomplete code, a semicolon would prevent it from error highlighting un-necessary lines (and placing red squigglies everywhere). Other recovery tokens generally include "things that you can only see on the left hand side", like "let", or "const". Those other recovery tokens are usually common enough that overhighlighting errors tends to not be much of a problem anyway, but a semicolon is still a useful tool in that regard.


Conclusion

To conclude I could probably get used to not having them, but I also see no need to get rid of them.

We're already changing quite a lot of things, so I'm not sure if we should add this to the list, as it's mostly a stylistic change, but people have to put effort into:

  1. getting used to the style, reading code this way
  2. understanding the rules on semi's (which is not a bad thing, but should be taken into account)

And that second one is worrying me, that preventable errors can still slip in.

So I would suggest only changing this when the (big) majority agrees we should get rid of them, and everyone understands the effort (and risk).

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.

2 participants