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

Switch from react-scripts (Create React App) to Vite #2462

Merged
merged 32 commits into from
Oct 23, 2024
Merged

Switch from react-scripts (Create React App) to Vite #2462

merged 32 commits into from
Oct 23, 2024

Conversation

jake-low
Copy link
Contributor

@jake-low jake-low commented Oct 10, 2024

Create React App seems to be effectively abandoned. It hasn't had a release in over two years, and has fallen far behind the latest versions of its major dependencies (babel, eslint, jest, postcss, tailwind, webpack). I looked but couldn't find an active fork of the project.

It seems like the "batteries included" build tool that people are using these days is Vite. So I took a stab at porting MapRoulette to use Vite (and its companion test framework, Vitest) instead of react-scripts and Jest.

It actually wasn't as difficult as I thought it might be. Vite's docs are pretty good, and mostly it works the way you'd want it to out of the box, so the configuration file ended up just being a dozen lines or so.

In addition to no longer being tied to an unmaintained project, some other benefits of switching to Vite are:

  • faster builds (35s vs 72s on my machine) - Vite uses ESBuild which is much faster than Babel
  • faster test runs (60s vs 170s) - Vitest uses multiple processes to run tests in parallel
  • slightly smaller main bundle (5.0M vs 6.6M), I think because dependencies are now loaded as ESM when available which results in better tree-shaking

The developer experience with Vite is mostly the same. A few differences worth noting are:

  • Vite requires files that use JSX syntax to have a .jsx (or .tsx) extension
  • To access environment variables, you use import.meta.env instead of process.env
  • Vite writes the bundled code out to dist/, not build/

Other than that it's pretty much the same. yarn {start,build,test} work like before. Source maps still work. Localization files work the same. .env files work the same.

@jake-low
Copy link
Contributor Author

Here's the npm trends graph for react-scripts vs vite, just for fun.

image

@jake-low jake-low marked this pull request as ready for review October 14, 2024 22:10
@jake-low
Copy link
Contributor Author

This is now working, tested, rebased, and ready for review.

Note that I've split this upgrade out into a whole bunch of commits, in case that makes reviewing easier, but I'll be squashing them all together at merge time because some of the intermediate commits don't build, or have failing tests.

@CollinBeczak
Copy link
Collaborator

CollinBeczak commented Oct 15, 2024

I think there is something wrong with the handling of oauth... I am unable to log in.

whoami:
{
    "status": "NotAuthorized",
    "message": "Authorization failed (server replied with a 401). This can happen if the consumer key was not correct or the signatures did not match."
}

@CollinBeczak
Copy link
Collaborator

Other than that, i had no problem running the app and everything else is working great!

@jake-low
Copy link
Contributor Author

I think there is something wrong with the handling of oauth... I am unable to log in.

Thanks for testing and catching this. I think I've fixed this now. Running with a local backend on port 9000 works for me now, using this .env.development.local file:

REACT_APP_URL='http://127.0.0.1:3000'
REACT_APP_SERVER_OAUTH_URL='http://127.0.0.1:9000/auth/authenticate?redirect=http://127.0.0.1:3000'
REACT_APP_FEATURE_CHALLENGE_ANALYSIS_TABLE='enabled'
REACT_APP_FEATURE_MOBILE_DEVICES='enabled'

The cause of the issue was that I'd messed up a search-and-replace on process.env.NODE_ENV, which the authentication callback code uses to trigger some special behavior for local development. My changes had accidentally prevented this necessary behavior from running, but that should now be resolved.

@CollinBeczak
Copy link
Collaborator

Yup, oauth is working now for me. There is one more thing that should be investigated/fixed soon after or before merging this...
The initial loading of the app is slower, at least in local developement, and there are some links within the app that seem to, for lack of better words, unmount the vite application, causing that long initial load to happen again:

Screen.Recording.2024-10-15.at.3.49.07.PM.mov

As you can see in the video, the points link to the leaderboard seems to open a new instance of the application causing the long initial load to happen again, this is much more apparent in the vite app.

@CollinBeczak
Copy link
Collaborator

CollinBeczak commented Oct 15, 2024

I assume users would be fine with the initial load taking longer for now... maybe we can add a placeholder till the app is fully loaded so that its not a blank white screen, but in the meantime i feel like making sure all the links in the app dont open another instance is a good first step.

@jake-low
Copy link
Contributor Author

It looks like that particular link is a plain old HTML <a> element, so clicking it causes a full page refresh rather than a client-side navigation change (you'd need to use <Link> for that behavior). This happens in the current version of the app too: on maproulette.org, clicking the points ticker is slower than clicking the "Leaderboard" link in the header, even though they both go to the same page.

The Vite app seems like it's slower than react-scripts was at handling these full page reload events (it might be faster if you close the devtools? having them open sometimes disables caching). But that is only true of the developer experience with Vite. Full page loads of the production build of the app should be pretty fast.

Nonetheless, there's no reason for the link in question to be an <a> instead of a react-router <Link> element; I'll fix that.

@jake-low jake-low merged commit 7c3ae78 into main Oct 23, 2024
6 checks passed
@jake-low jake-low deleted the jlow/vite branch October 23, 2024 04:57
@CollinBeczak CollinBeczak mentioned this pull request Nov 12, 2024
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.

2 participants