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

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

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
import * as path from 'path'
import * as ssri from 'ssri'
import { fileURLToPath } from 'url'
import { defineConfig, loadEnv } from 'vite'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { defineConfig, loadEnv } from 'vite'
import { defineConfig } from 'vite'

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
3 participants