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

Migrate from Webpacker to ViteJS #24981

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Migrate from Webpacker to ViteJS #24981

wants to merge 4 commits into from

Conversation

renchap
Copy link
Sponsor Member

@renchap renchap commented May 14, 2023

Warning: this is a big work in progress with a lot of things not working.

Is it based on my #24906 PR, so ignore Intl-related commits for now.

With this change, the frontend toolchain is managed by Vite, a fast Javascript bundler based on esbuild. It brings a lot of improvements, including support for all modern JS features, much more efficient code-splitting, far less required packages (from 1669 to 1139 at my last count) and a much improved performance.

Todo:

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

Copy link
Contributor

@ThisIsMissEm ThisIsMissEm left a comment

Choose a reason for hiding this comment

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

Might be worth adding the following to .gitattributes:

*.tsx.snap linguist-generated
*.jsx.snap linguist-generated

This will make the snapshots be collapsed by default when review the PRs.

vite.config.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@ThisIsMissEm ThisIsMissEm left a comment

Choose a reason for hiding this comment

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

This is looking good, left comments on a few things I noticed when looking through it, though I know it's not fully ready yet.

.github/dependabot.yml Outdated Show resolved Hide resolved
Gemfile.lock Outdated
Comment on lines 899 to 905

RUBY VERSION
ruby 3.2.2p53

BUNDLED WITH
2.4.12
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is normally not committed? (though I personally think it should be)

app/javascript/entrypoints/application.js Outdated Show resolved Hide resolved
app/javascript/mastodon/features/emoji/emoji_compressed.js Outdated Show resolved Hide resolved
app/javascript/mastodon/load_locale.js Outdated Show resolved Hide resolved
app/views/admin/accounts/index.html.haml Outdated Show resolved Hide resolved
app/views/auth/sessions/two_factor.html.haml Outdated Show resolved Hide resolved
app/views/shared/_web_app.html.haml Outdated Show resolved Hide resolved
@renchap
Copy link
Sponsor Member Author

renchap commented May 19, 2023

Might be worth adding the following to .gitattributes:

*.tsx.snap linguist-generated
*.jsx.snap linguist-generated

This will make the snapshots be collapsed by default when review the PRs.

I am not sure this is a good idea, you might want to review any diffs in snapshots to ensure the snapshot did not get updated to something not expected?

@github-actions
Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

@renchap
Copy link
Sponsor Member Author

renchap commented May 20, 2023

With the removal of Webpacker, Jest (and therefore most of Babel), we go from 1670 node packages to less than 1000 🎉

Slowly moving forward with migrating all the special usecases

  • Locale files are now loaded properly, ignoring the ones we dont need
  • Images should load properly (more testing is needed here)
  • The share page load correctly
  • Output files are compressed using gzip and brotli, like the current Webpack setup
  • I added some custom code to include the parent's folder name when possible in the output filename, otherwise there were a lot of unhelpful index-smthg.js

Some assets were using media/… paths, I am not sure why, they are now treated like the other assets.

I still need to ensure every other entrypoint is loading correctly.

The locale file should be manually pre-loaded as we know the locale server-side, should should not be hard to add.

I added a way to visualise the output size through a RollUp plugin:
image

It shows that the emoji data files are bundled in the history.js chunk, which is weird. I need to detangle all this emoji-picker code, and ensure it is loaded only when needed using a dynamic import. It should not be this hard, using <Suspense> I think.

But the emoji-related code is a real mess, and it would be very welcome to update to a recent and supported Emoji-mart, and remove all this weird emoji handling code.

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2023

This pull request has resolved merge conflicts and is ready for review.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2023

This pull request has merge conflicts that must be resolved before it can be merged.

@mjankowski
Copy link
Contributor

I was looking around at blockers to updating Node and stumbled across this one ... I wonder if there's a more incremental way to do things here? Migrating one piece at a time instead of bigger replacement?

The only thing that comes to mind here would be to try to isolate the various areas (admin web pages, user settings pages, the main react app, etc) and/or also isolate the assets, stylesheets, images, fonts, etc -- and try to go piece by piece there instead of all at one.

I can open a POC PR along those lines if you think that might nudge this forward.

@renchap
Copy link
Sponsor Member Author

renchap commented Aug 23, 2023

We could try to migrate entrypack per entrypack, but it is quite limited because of the shared code between them, which may need to be adapted from Vite, or handle both Vite & Webpack, which can be complex.

I stopped my work on this because it will not be mergeable until 4.2 is released, but I still intent to have it all migrated.

I could extract the migration from jest to vitest, it might make sense and reduce the overall diff a bit.

@github-actions
Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

@renchap renchap force-pushed the vite branch 4 times, most recently from b56cbc8 to 8e7cb56 Compare April 23, 2024 08:30
@renchap
Copy link
Sponsor Member Author

renchap commented Apr 23, 2024

Some more updates:

  • the service worker has been converted to work with Vite. It should behave the same, with Workbox as a preloader/cache layer (is this really needed? Unsure, but its there)
  • babel-plugin-preval is no longer used to execute a JS file at compilation time and bundle its result in the service worker (for only packing the needed locales), I switched it to a custom vite plugin which does the same thing though a virtual module. Note: this could be used to remove preval entirely later for the 2 remaining emoji-related files
  • app/javascript/packs is renamed to /app/javascript/entrypoints in a first commit while still using webpacker, which we should be able to extract in a separate PR
  • the production output is kept in public/packs, so there is no need to change nginx configuration or any specific server/CDN config

Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

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.

None yet

4 participants