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

webpack -> rspack migration #388

Merged
merged 10 commits into from
Oct 3, 2024
Merged

webpack -> rspack migration #388

merged 10 commits into from
Oct 3, 2024

Conversation

Theodlz
Copy link
Collaborator

@Theodlz Theodlz commented Sep 30, 2024

Bundling times for SkyPortal (in production mode for example) are cut in half using rspack, and it looks like the bundle sizes are also slightly reduced in production mode.

Overall rspack seems to offer us the same features as webpack, while speeding up things which is very handy while developing, and while running any CI/CD.

I'd be in favor of moving to rspack, especially given how minimal the changes needed are.

@stefanv
Copy link
Contributor

stefanv commented Sep 30, 2024

If it's a 1-to-1 replacement, that's fine; we have a somewhat complex webpack configuration, so I wanted to make sure that it takes all behavior into consideration.

@Theodlz
Copy link
Collaborator Author

Theodlz commented Sep 30, 2024

If it's a 1-to-1 replacement, that's fine; we have a somewhat complex webpack configuration, so I wanted to make sure that it takes all behavior into consideration.

yep! That's why I made this one a draft. I want to make sure that we don't leave a bunch of boilerplate webpack stuff that we don't need for rspack, and obviously that we don't miss anything that rspack silently ignores. I'm running some stuff locally right now

@Theodlz Theodlz mentioned this pull request Oct 1, 2024
@Theodlz
Copy link
Collaborator Author

Theodlz commented Oct 1, 2024

@stefanv I had to edit the CI to point to a custom branch of the template app (that will be its own PR), but CI is running and everything is working as expected.

I checked the config, and rspack runs some pretty strict validation, which should make it error out if the config contains things that are not supported, instead of ignoring it. So, this should mean that our config is valid. I looked through the rest of the doc, and none of our current config isn't supported. You can see that here: https://rspack.dev/guide/migration/webpack#updating-configuration

@Theodlz
Copy link
Collaborator Author

Theodlz commented Oct 1, 2024

let me know when/if you are happy with this, and I'll remove the temporary changes, and we can look at the PR on the template app.

@Theodlz Theodlz marked this pull request as ready for review October 1, 2024 15:24
@Theodlz Theodlz requested a review from stefanv October 1, 2024 15:24
@Theodlz
Copy link
Collaborator Author

Theodlz commented Oct 1, 2024

cesium-ml/baselayer_template_app#304 Template App PR is ready

skyportal/skyportal#5278 SkyPortal PR is ready, can be merged once baselayer's is.

@stefanv
Copy link
Contributor

stefanv commented Oct 1, 2024

I'm happy to give this a try. Correct order seems to be: baselayer template app, then updated version of this, then skyportal.

I think it would be good to have a note somewhere that we use rspack as a 1:1 replacement of webpack, in case other projects prefer to use that.

@Theodlz
Copy link
Collaborator Author

Theodlz commented Oct 2, 2024

I'm happy to give this a try. Correct order seems to be: baselayer template app, then updated version of this, then skyportal.

I think it would be good to have a note somewhere that we use rspack as a 1:1 replacement of webpack, in case other projects prefer to use that.

That's a great idea. Would you rather have me add this to baselayer, or the template app that has the rspack.config.js?

@stefanv
Copy link
Contributor

stefanv commented Oct 2, 2024

Baselayer docs, maybe, since that's where the command is being invoked from?

@Theodlz
Copy link
Collaborator Author

Theodlz commented Oct 3, 2024

@stefanv would you rather have me merge baselayer pointing to the edited template app (for the CI to be green on main), then merge the PR on the template app, then open another one on baselayer to go back to the main of the template app?

Or, I get this PR to go back to the main branch of the template app, we merge it and CI fails, but once the template app's PR is merged we just rerun the CI here to have it be green.

Any preference? I feel like the second option is best, since I just need to rerun an action rather than re-open a PR.

@Theodlz
Copy link
Collaborator Author

Theodlz commented Oct 3, 2024

adding the docs changes in a minute btw

@Theodlz
Copy link
Collaborator Author

Theodlz commented Oct 3, 2024

Baselayer docs, maybe, since that's where the command is being invoked from?

done, I added a short paragraph/section dedicated to the web app bundling microservice

run: |
git clone https://github.com/cesium-ml/baselayer_template_app
git clone -b rspack-v1 https://github.com/Theodlz/baselayer_template_app
Copy link
Contributor

Choose a reason for hiding this comment

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

Can merge as soon as this has returned to cesium-ml

@Theodlz Theodlz requested a review from stefanv October 3, 2024 21:09
@stefanv stefanv merged commit 8f6d1dc into cesium-ml:main Oct 3, 2024
3 checks passed
@stefanv
Copy link
Contributor

stefanv commented Oct 3, 2024

Thanks!

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