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

feat: use vite instead of react-app-rewired #8924

Merged
merged 79 commits into from
Mar 6, 2025
Merged

feat: use vite instead of react-app-rewired #8924

merged 79 commits into from
Mar 6, 2025

Conversation

NeOMakinG
Copy link
Collaborator

@NeOMakinG NeOMakinG commented Feb 21, 2025

Description

What a pull request! It was initially supposed to be a Spike, but spiking something like this is super hard, it's like a coin flip. It took me around 5 hours to figure out that we had no blockers on this.

Here is the steps I did follow:

  • Added a basic vite configuration
  • Seen what was the blockers to make it work
    • Some bundles have to be split from the main bundle as our hosting is supporting 26mb by file max
    • Added different resolutions for some packages we are importing, we might be able to avoid them at some point but I couldn't find a proper way yet
    • As we are using esmodules, sometime I had to switch from require to import, not sure if we can avoid this, if anyone as an idea, is it a blocker?
    • I had to change root paths to '@', previously we were using nothing as a root path, but it was causing conflicts where vite was searching for a node module for paths with only one word, happy to revert if we find better solutions but I feel like it's also better to have a keyword for the root path
  • Then finally the config was working for both build and dev environment, I had to make it ISO with our past configuration:
    • Minify if production, or don't do it if development mode
    • Source map if dev mode, don't if it's production
    • Fix tests that were failing after the tsconfig changes and whatever

To be discussed: Do we want to bring back circular dependencies plugin? Because the vite plugin is detecting around 15 more circular dependencies, can be done in a follow up but as discussed with @gomesalexandre we might not care

Issue (if applicable)

closes #8812

Risk

High (Effectively high as we touch absolutely the whole app)

High Risk PRs Require 2 approvals

What protocols, transaction types, wallets or contract interactions might be affected by this PR?

Testing

Smoke test absolutely everything in the app

Engineering

n/a

Operations

n/a

  • 🏁 My feature is behind a flag and doesn't require operations testing (yet)

Screenshots (if applicable)

Create and connect native wallet

https://jam.dev/c/f2660871-45bc-48b4-a237-9bafe28a86af

Import native wallet

https://jam.dev/c/4188921f-8ad7-46b0-8895-be6b972d45f9

General test + swap

https://jam.dev/c/9d841328-482c-427e-b716-c256d22ca816

also tested on gome for mobile

@NeOMakinG NeOMakinG requested a review from a team as a code owner February 21, 2025 17:28
@NeOMakinG NeOMakinG changed the title feat: use vite instead of react-app-rewiared feat: use vite instead of react-app-rewired Feb 21, 2025
@NeOMakinG
Copy link
Collaborator Author

8e0f2a4

Changed yarn preview to yarn serve and added yarn preview:prod and yarn preview:dev as commands to build and serve correct envs in 8e0f2a4

Other commits are residual changes to make it work properly

Other errors you did mention should be fixed at some points in our flow and not a part of this work as you mentioned, should we create an issue to track them?

Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NeOMakinG NeOMakinG enabled auto-merge (squash) March 6, 2025 09:27
@NeOMakinG NeOMakinG merged commit 1f00c36 into develop Mar 6, 2025
3 checks passed
@NeOMakinG NeOMakinG deleted the vite-spike branch March 6, 2025 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vite Spike
5 participants