Skip to content

Conversation

@peaBerberian
Copy link
Collaborator

@peaBerberian peaBerberian commented Aug 19, 2024

Overview

In this PR, I attempt to reduce the reliance on eslint (specifically, as compared to potential other linters) in the code:

  • I removed statements on top of test files which disabled many rules, in particular those preventing us from relying on TypeScript's any type .

    Turns out relying on the true type is not that inconvenient (just one import statement away), may lead to better-written tests, and won't impact vitest's mocking as type-import are erased by TypeScript at compile-time anyway.

  • I tried to rely on eslint-disable-next-line instead of multi-lines eslint-disable, as some linting tools don't support the latter and as it more precizely pinpoint the area of the code needing the exception.

  • I removed eslint statements from the scripts directory as it doesn't seem to be checked for now.

The idea is to facilitate potential future switches to other linters.

Reasoning

After trying to update eslint in #1506, we realized that we're globally dissatisfied with the tool:

  • We don't need all its complexity or extensions as we're not an usual big front-end applications relying on some framework, and we generally tend to not wanting to be too strict on sometimes frequently-changing idioms (e.g. iterator methods vs for loops, ternary vs if-else, barrel files vs no barrel files etc.).

    Here the issue is that eslint configuration and dependencies had become too complex (or perhaps it always was), maybe for the sake of modularity and being powerful - leading us to always spend hours each time we want to change any minor thing about the tool, including updating it.

    Yet we mostly use it now as a static analysis tool to potentially catch bugs (e.g. like we use sonarcloud) - both present (right now in the code) and future (e.g. an unclear pattern that may lead to issues in the future without us realizing). In that regard, some other tools (biome, oxlint) seem to also be more or less be adapted to our needs or at least clearly approach them.

    As a huge advantage, those tools seems to be much easier to setup, seem to produce much clearer errors when misusing it and also provide more complete solutions when issues are detected, saving us time on a tool we don't like to spend too much time on.

  • It's been pretty slow, leading me and perhaps others to just let the CI do the checking many time, just to then squash commits and force-push, which is never too pleasant (as it takes time and is not exactly a fun activity).

    Also here, alternatives seems to present much better performances: biome for example takes 200ms on my PC to lint the src directory, wheras eslint take 18 seconds (!).

So we're seriously considering to at least look at alternatives, and see if they fulfill our needs. In that context, we want to make sure that switching between linting tools is not too painful.

@peaBerberian peaBerberian force-pushed the misc/de-eslint-code branch 4 times, most recently from 1fc20cc to f5a431b Compare August 19, 2024 17:14
@peaBerberian peaBerberian added code Priority: 3 (Low) This issue or PR has a low priority. labels Aug 19, 2024
@peaBerberian peaBerberian changed the title [WIP] Limit reliance on eslint in the RxPlayer's code Limit reliance on eslint in the RxPlayer's code Aug 19, 2024
@peaBerberian peaBerberian force-pushed the misc/de-eslint-code branch 5 times, most recently from dc45873 to 3bae868 Compare August 20, 2024 08:36
@peaBerberian peaBerberian added this to the 4.2.0 milestone Sep 4, 2024
Overview
--------

In this commit, I attempt to reduce the reliance on eslint in the code:

  - I removed statements on top of test files which disabled many rules,
    in particular those preventing us from relying on TypeScript's
    `any` type .

    Turns out relying on the true type is not that inconvenient (just
    one import statement away), may lead to better-written tests, and
    won't impact vitest's mocking as type-import are erased by
    TypeScript at compile-time anyway.

  - I tried to rely on `eslint-disable-next-line` instead of multi-lines
    `eslint-disable`, as some linting tools don't support the latter and
    as it more precizely pinpoint the area of the code needing the
    exception.

  - I removed eslint statements from the `scripts` directory as it
    doesn't seem to be checked for now.

The idea is to facilitate potential future switches to other linters.

Reasoning
---------

After trying to update eslint in #1506, we realized that we're globally
dissatisfied with the tool:

  - We don't need all its complexity or extensions as we're not an usual
    big front-end applications relying on some framework, and we
    generally tend to not wanting to be too strict on sometimes
    frequently-changing idioms (e.g. iterator methods vs for loops,
    ternary vs if-else, barrel files vs no barrel files etc.).

    Here the issue is that eslint configuration and dependencies had
    become too complex (or perhaps it always was), maybe for the sake
    of modularity and being powerful - leading us to always spend hours
    each time we want to change any minor thing about the tool,
    including updating it.

    Yet we mostly use it now as a static analysis tool to potentially
    catch bugs (e.g. like we use sonarcloud) - both present (right now
    in the code) and future (e.g. an unclear pattern that may lead to
    issues in the future without us realizing).
    In that regard, some other tools (biome, oxlint) seem to also be
    more or less be adapted to our needs or at least clearly approach
    them.

    As a huge advantage, those tools seems to be much easier to setup,
    seem to produce much clearer errors when misusing it and also
    provide more complete solutions when issues are detected, saving us
    time on a tool we don't like to spend too much time on.

  - It's been pretty slow, leading me and perhaps others to just let
    the CI do the checking many time, just to then squash commits and
    force-push, which is never too pleasant (as it takes time and is not
    exactly a fun activity).

    Also here, alternatives seems to present much better performances:
    biome for example takes 200ms on my PC to lint the `src` directory,
    wheras eslint take 18 seconds (!).

So we're seriously considering to at least look at alternatives, and see
if they fulfill our needs. In that context, we want to make sure that
switching between linting tools is not too painful.
@peaBerberian peaBerberian merged commit 19c66e6 into dev Sep 4, 2024
peaBerberian added a commit that referenced this pull request Sep 2, 2025
An issue, #1736, noticed
that we have an issue with the `getPeriodForTime` util which links time
and Period.

The way the util was written means it had to be looped over from the
last to the first, but I don't know why I decided to replace it with a
fancier `for...of` (which goes from first to last) in a totally
unrelated refacto (#1507).

This broke the logic and lead in conditions that are hard to predict
infinite rebuffering issues.

I also made some other code more resilient here, because I thought that
I saw another issue first. I don't really know if that other case is
possible but I thought that it didn't cost anything to be resilient
there.
peaBerberian added a commit that referenced this pull request Sep 29, 2025
An issue, #1736, noticed
that we have an issue with the `getPeriodForTime` util which links time
and Period.

The way the util was written means it had to be looped over from the
last to the first, but I don't know why I decided to replace it with a
fancier `for...of` (which goes from first to last) in a totally
unrelated refacto (#1507).

This broke the logic and lead in conditions that are hard to predict
infinite rebuffering issues.

I also made some other code more resilient here, because I thought that
I saw another issue first. I don't really know if that other case is
possible but I thought that it didn't cost anything to be resilient
there.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Priority: 3 (Low) This issue or PR has a low priority.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants