Add "no-implicit-coercion"

[juliangruber]

https://eslint.org/docs/rules/no-implicit-coercion

I find this rule useful as it prevents shorthand syntax that can be either difficult to read or understand.

To me it's always better to write

new Date().getTime()

instead of

+new Date()

or

String(foo)

instead of

`${foo}`

There are more characters on screen yes, but the intend is clearer and reading the code requires less understanding of JavaScript internals.

I know that @kvz and @tim-kos like using these syntax tricks a lot, so I don't expect this to go very easy.

[tim-kos]

Fine with me. I won‘t fight over this one. :) Kind regards, Tim Co-Founder Transloadit @tim_kos

…

Am 22.04.2022 um 15:02 schrieb Julian Gruber ***@***.***>:  https://eslint.org/docs/rules/no-implicit-coercion I find this rule useful as it prevents shorthand syntax that can be either difficult to read or understand. To me it's always better to write new Date().getTime() instead of +new Date() or String(foo) instead of `${foo}` There are more characters on screen yes, but the intend is clearer and reading the code requires less understanding of JavaScript internals. I know that @kvz and @tim-kos like using these syntax tricks a lot, so I don't expect this to go very easy. You can view, comment on, or merge this pull request online at: #15 Commit Summary 51ea8c8 Enable "no-implicit-coercion" dac55af Disallow template shorthand string conversion 4523f22 Move to its own section File Changes (1 file) M index.js (7) Patch Links: https://github.com/transloadit/eslint-config-transloadit/pull/15.patch https://github.com/transloadit/eslint-config-transloadit/pull/15.diff — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

[mifi]

I usually use !!val, but I can live with having to write Boolean(). I think some of these shorthands are very cryptic so it makes sense to not allow them.

[kvz]

Probably not one I would have proposed myself no, but also not one I am religious about to keep out of our standard, so I am fine personally with merging this.

I do think the Uppy team should weigh in first tho. And also I think this will add yet another thousand warnings to our big repos. At one point we should make a push to also fix a few of those. Maybe a few heavy hitters can be addressed with jscodemods?

[juliangruber]

Maybe a few heavy hitters can be addressed with jscodemods?

I'm assuming we can take care of 99% of cases using string search/replace, or even eslint --fix if that covers it.

[aduh95]

To me it's always better to write

new Date().getTime()

instead of

+new Date()

FWIW, it's always better (performance-wise, and imho in general) to write Date.now() 😉

[aduh95]

Same as Kev. More consistency in the code we write and review is ultimately a good thing imo

[juliangruber]

FWIW, it's always better (performance-wise, and imho in general) to write Date.now() 😉

I agree about performance, but not maintainability. When you're not in a hot code path then a Date object is more useful as it allows access to the timestamp and all the other date functions too.

[Murderlon]

Don't have a strong opinion on this. But we already have 2000+ warnings in the Uppy repo and ideally we get it down rather than up, because we are just getting too used to seeing warnings.

[aduh95]

Don't have a strong opinion on this. But we already have 2000+ warnings in the Uppy repo and ideally we get it down rather than up, because we are just getting too used to seeing warnings.

FWIW the goal is to get as close as 0 warnings when doing the ESM transition, and then change ESLint settings from warning to error on all rules.

[goto-bus-stop]

@kvz consider my peace held 😇

[kvz]

We'll need to trial this as a warning to give projects the chance to upgrade, and once there are no more warnings, then we promote to error.

[juliangruber]

sgtm

[aduh95]

Counter proposal: I think we should keep it as an error, projects should override that rule to be a warning instead until they've fixed all the occurrences. (please no more warnings 🥺)

[arturi]

I am all for readability, !!var makes my brain go crazy for 5 minutes at least, and I have not seen +new Date() before this PR. Green light.

[juliangruber]

Thank you everyone for your input!

@kvz what are the next steps?

[kvz]

1. collecting more ideas for v3 (if no more linting changes, upgrading eslint & friends is always a good idea)
2. push out v3.0.0-0
3. trial it in uppy & api to see if we run into any unforeseen warning/error/autofix, and if tests pass and so on
4. if we are happy, push out v3.0.0
5. upgrade all consuming repos (for inspiration see Roll out v2.0.0-0 across main js repos #13, also the hint at the bottom of the description there for helping rolling out big linting upgrades)

[juliangruber]

1. collecting more ideas for v3 (if no more linting changes, upgrading eslint & friends is always a good idea)

To me the smaller the change the better, as it will be easier to upgrade. Upgrading dependencies sounds great 👍