DakaraProject / dakara-client-web

Web client for the Dakara Project
MIT License
5 stars 1 forks source link

Vite migration #154

Open NextFire opened 9 months ago

NextFire commented 9 months ago

Migrate the project to Vite + swr (in replacement of CRA + webpack).

eslint/stylecheck configurations are unmodified but have been moved out of package.json.

odrling commented 8 months ago

Poke for comments @Neraste

NOTE: The diff is ridiculously massive but there is little change in the code. Notably it doesn't have more conflicts than the current develop branch with the other active PR #141.

This is mainly a QoL change, installs and builds are faster.

Neraste commented 8 months ago

Thanks for the proposal. What are the advantages to change the build worflow (again)?

odrling commented 8 months ago

As I mentioned npm ci is faster now (18s → 7s on a quick test) and more importantly builds are also a lot faster. Here are some raw build times benchmarks from my laptop:

❯ hyperfine 'npm -C old run build' 'npm -C new run build'
Benchmark 1: npm -C old run build
  Time (mean ± σ):     20.692 s ±  0.850 s    [User: 37.340 s, System: 3.223 s]
  Range (min … max):   19.879 s … 22.741 s    10 runs

Benchmark 2: npm -C new run build
  Time (mean ± σ):      7.076 s ±  0.095 s    [User: 12.421 s, System: 1.851 s]
  Range (min … max):    6.959 s …  7.225 s    10 runs

Summary
  npm -C new run build ran
    2.92 ± 0.13 times faster than npm -C old run build

So again I would assume this also translates into better dev experience when working on the project.

Neraste commented 8 months ago

I remember CRA was notoriously impossible to configure without ejecting the project (this is why I used CRA rewired or something). If the configuration is easier with vite, that would be interesting. For the build speed, it's always appreciable, but I don't remember it was a real issue. I'll try it on my machine.

Neraste commented 8 months ago

I just tested it. It seems to work nice, but in dev mode, vite eats all my memory and crashes after 1 minute:


  VITE v5.0.12  ready in 602 ms

  ➜  Local:   http://localhost:3000/
  ➜  Network: use --host to expose
  ➜  press h + enter to show help
[Stylelint] Found 0 error and 0 warning
node:events:505
      throw er; // Unhandled 'error' event
      ^

Error [ERR_WORKER_OUT_OF_MEMORY]: Worker terminated due to reaching memory limit: JS heap out of memory
    at new NodeError (node:internal/errors:372:5)
    at [kOnExit] (node:internal/worker:276:26)
    at Worker.<computed>.onexit (node:internal/worker:198:20)
Emitted 'error' event on Worker instance at:
    at [kOnExit] (node:internal/worker:276:12)
    at Worker.<computed>.onexit (node:internal/worker:198:20) {
  code: 'ERR_WORKER_OUT_OF_MEMORY'
}

Node.js v18.0.0

I saw some issues related to this problem on the vite repo.

NextFire commented 8 months ago

What are the advantages to change the build worflow (again)?

As @odrling said, Vite (and swr) introduces a lot of QoL and CI improvements, notably less dependencies overall, faster npm ci, npm run dev start, npm build and smaller resulting builds.

If the configuration is easier with vite, that would be interesting.

All the configuration happens in vite.config.js. I tried to match the most the previous CRA config.

I also added vite-plugin-checker which is a Vite plugin that reports all the linting warnings and errors in the browser.

I just tested it. It seems to work nice, but in dev mode, vite eats all my memory and crashes after 1 minute

Can you reproduce it on the latest LTS (node 20)?

I also found this issue on the vite-plugin-checker repo which leads me to believe that the 234 eslint errors reported on our project may be a little too overwhelming for it. Can you try again after removing the plugin from vite.config.js?

Neraste commented 8 months ago

Seems better with node 20.

There are a lot of linter errors (246) and warnings (14), they didn't show up before. We cannot leave it like that, but some of them seem inherent to (what I know on how to use) React. By instance, importing React in every file without using it, and assigning classes.

NextFire commented 8 months ago

Do you want me to fix these errors and warnings in this PR? Or in a separate one?

I didn't want to touch any existing React code in this PR so I let all the eslint errors and warnings.

NextFire commented 8 months ago

importing React in every file without using it

I think Vite imports it implicitly in every JSX file now.

and assigning classes

I am not familiar with the previous class-based components system but maybe we can ignore this error globally if all the assignments are actually safe.

Neraste commented 8 months ago

It would be nice to find a linter config tailored for React, to begin with.

NextFire commented 8 months ago

I already merged the Vite default React config with your custom plugins and rules.

Neraste commented 8 months ago

It's more about the eslint/stylecheck configuration.

Neraste commented 8 months ago

Do you want me to fix these errors and warnings in this PR? Or in a separate one?

I didn't want to touch any existing React code in this PR so I let all the eslint errors and warnings.

Sounds fair to do it in another PR. Please create an issue for that.

Neraste commented 7 months ago

@NextFire I've this branch (#156) that induces a lot of refactor and I'd like to merge it before this one. How pissed off would you be to handle the merging?

NextFire commented 7 months ago

154 should be quite easy to rebase as it doesn't touch any React file. #155 may be another story. Go ahead and merge your branch, I'll figure out how to handle it.