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

Vite migration #154

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from
Open

Vite migration #154

wants to merge 15 commits into from

Conversation

NextFire
Copy link
Member

@NextFire NextFire commented Feb 1, 2024

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
Copy link

odrling commented Feb 16, 2024

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
Copy link
Member

Neraste commented Feb 24, 2024

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

@odrling
Copy link

odrling commented Feb 24, 2024

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
Copy link
Member

Neraste commented Feb 24, 2024

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
Copy link
Member

Neraste commented Feb 24, 2024

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
Copy link
Member Author

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
Copy link
Member

Neraste commented Feb 24, 2024

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
Copy link
Member Author

NextFire commented Feb 24, 2024

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
Copy link
Member Author

NextFire commented Feb 24, 2024

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
Copy link
Member

Neraste commented Feb 24, 2024

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

@NextFire
Copy link
Member Author

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

@Neraste
Copy link
Member

Neraste commented Feb 25, 2024

It's more about the eslint/stylecheck configuration.

@Neraste
Copy link
Member

Neraste commented Feb 25, 2024

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
Copy link
Member

Neraste commented Mar 24, 2024

@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?

jsconfig.json was loaded by editors such as vscode so their LSP could resolve aliases
@NextFire
Copy link
Member Author

NextFire commented Mar 24, 2024

#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.

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.

None yet

3 participants