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 webpacker to esbuild #203

Closed
wants to merge 1 commit into from

Conversation

baarkerlounger
Copy link
Contributor

@baarkerlounger baarkerlounger commented Jan 10, 2022

What is Esbuild?
tl:dr an alternative to webpack, i.e. a Javascript bundler written in Go that's significantly faster. https://blog.logrocket.com/fast-javascript-bundling-with-esbuild/

This PR migrates us from using Webpacker to using Esbuild. See this for more information on the general direction of bundling JS in rails https://world.hey.com/dhh/rails-7-will-have-three-great-answers-to-javascript-in-2021-8d68191b.

The advantages are primarily speed: (Test suite for example runs in ~44 s vs ~90s) and it's a lot less code.

Downsides are it may be less well tested/documented, particularly across Gov repos and workflows are slightly different.

Do we need this at all?
Short answer: no. It's really just a case of which tool we prefer to work with and get the most benefit from.

Edit: While we still don't need it Webpacker is now officially considered "retired" by Rails upstream community so moving to at least js-bundling seems worthwhile long term. https://github.com/rails/webpacker#webpacker-has-been-retired-

Caveats:

This ditches Babel - possibly that means if our stimulus controllers use ES6 it may not work correctly on IE (which is unsupported by MS but we know from server logs some users are still on). Probably need to test what the experience would be like there. https://world.hey.com/dhh/modern-web-apps-without-javascript-bundling-or-transpiling-a20f2755

@baarkerlounger baarkerlounger force-pushed the migrate_webpacker_to_esbuild branch 2 times, most recently from 89d1166 to 99c2610 Compare January 10, 2022 14:09
Copy link
Contributor

@paulrobertlloyd paulrobertlloyd 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 all very curious, need to do some reading up!

app/assets/stylesheets/_accessible-autocomplete.scss Outdated Show resolved Hide resolved
@@ -1,23 +1,23 @@
.app-related-navigation {
@include govuk-text-colour;
@include all.govuk-text-colour;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooooh, what’s this? Is this syntax specific to the new bundling tool?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

app/assets/stylesheets/_tab-navigation.scss Outdated Show resolved Hide resolved
Copy link
Contributor

@dushan-madetech dushan-madetech left a comment

Choose a reason for hiding this comment

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

I have a few questions:

  1. How easy would it be to revert to using webpacker if we try this and find that it doesn't work for us?

  2. Should we document this swapping in an ADR? I'm asking because I like the PR description and want to it be visible without having to dig up this PR down the line.

  3. Since this isn't commonly used across Gov repos is there some way we can document/share our experiences with this so other Gov projects can benefit from our experiences?

Copy link
Contributor

@dushan-madetech dushan-madetech left a comment

Choose a reason for hiding this comment

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

Left some general questions but since this currently improves our test suite speed by reducing build times I'm happy to give this a go

@baarkerlounger baarkerlounger force-pushed the migrate_webpacker_to_esbuild branch 3 times, most recently from 4df7a20 to b916282 Compare January 19, 2022 12:49
@baarkerlounger baarkerlounger force-pushed the migrate_webpacker_to_esbuild branch 3 times, most recently from 47b81ca to 4bdec10 Compare January 28, 2022 12:27
@baarkerlounger baarkerlounger force-pushed the migrate_webpacker_to_esbuild branch 6 times, most recently from 8fab616 to 6dfeeed Compare February 11, 2022 09:22
@baarkerlounger baarkerlounger force-pushed the migrate_webpacker_to_esbuild branch 4 times, most recently from cae7b80 to a9ac431 Compare March 10, 2022 08:53
@baarkerlounger baarkerlounger force-pushed the migrate_webpacker_to_esbuild branch 3 times, most recently from a155e1c to 37b62bc Compare March 14, 2022 09:12
@baarkerlounger
Copy link
Contributor Author

It seems ES5/IE 11 isn't currently supported and there are no immediate plans to support it so this is unlikely to be an option for at the moment. evanw/esbuild#297.

Closing this for now.

@paulrobertlloyd
Copy link
Contributor

Farewell little buddy #203, you had a good run! 👋

@paulrobertlloyd
Copy link
Contributor

Do any of the new asset compilation options in Rails 7 help, or still early days for those?

@baarkerlounger
Copy link
Contributor Author

@paulrobertlloyd ☝️ for your evening reading enjoyment 😛

@baarkerlounger baarkerlounger deleted the migrate_webpacker_to_esbuild branch March 21, 2022 17:03
@baarkerlounger baarkerlounger restored the migrate_webpacker_to_esbuild branch March 21, 2022 17:03
@baarkerlounger baarkerlounger deleted the migrate_webpacker_to_esbuild branch June 30, 2022 11:07
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.

None yet

4 participants