Replace Webpacker with jsbundling-rails + Webpack

[aduth]

Related: #5725

Why: To improve performance of JavaScript builds, to adopt modern defaults for Rails, and to migrate away from the now >1 year out-of-date Webpack 4 tied to the latest stable version of Webpacker (and in so doing, eliminate such version tying altogether).

Observations:

Good:

• Build seems anecdotally faster
  • Still need to measure Edit: Replace Webpacker with jsbundling-rails + Webpack #5746 (comment)
  • Pulling out SASS compilation was a big part of the improvement
• Doesn't feel like a drastic departure
  • We already extensively customized the Webpack configuration anyways
  • Our handling of localization scripts already necessitated manual manifest lookups, so we already had most of the logic in ScriptHelper to do this part ourselves
• Drops a lot of unused dependencies from Webpacker, and makes our Webpack dependencies more explicit
  • This is also improved in Webpacker v6 by making most dependencies opt-in. But at this rate, I'm not sure if v6 will ever be released
• More flexibility for Webpack configuration
  • Specifically wondering if swapping from babel-loader to swc-loader could be a good move to further improve build performance

Not-so-good:

• The Webpack build itself is reasonably fast, but the Rails side of it can be rather slow, at least on initial boot-up Edit: This was addressed by bypassing Sprockets and outputting Webpack bundles directly to public.
  • I think this may be Sprockets processing or fingerprinting these files
    • Maybe we should disable digest in development?
    • Pending improvements to Sprockets could let us more easily opt-out of Sprockets digesting for already-digested file names
• Webpacker was better integrated into Rails requests and had some nice quality-of-life features we'd lose: Edit: This was addressed by running webpack-dev-server in parallel to the Rails process, and loading scripts as served from this dev server.
  • Automatic refresh on JavaScript file changes
  • Waiting for the Webpack compilation to finish before letting the response be presented
• rails s will no longer include JavaScript compilation. You'd also need to run yarn build --watch in the background

Next:

A few of the included changes are things we could probably consider doing sooner than later, regardless of jsbundling-rails:

• Stop compiling SASS with Webpack Edit: This was done separately in Standardize SASS processing to Sprockets #5793
• Prepare @18f/identity-i18n-webpack-plugin for Webpack 5
  • Webpack v5 uses Set in many places where they are arrays in Webpack v4. This causes deprecation warnings. In most cases, we could cast those values to arrays ([...val]) which would work for either a set or an array.

[zachmargolis]

Looked like CI was upset that webpacker.yml was removed so I pushed 3683f08 to handle that

[aduth]

Couple more thoughts:

• We could avoid the Sprockets fingerprinting overhead by bypassing Sprockets altogether and dumping the output to public.
  • Basically what Webpacker does.
• On this thought and on closer inspection, I'm not really sure that jsbundling-rails does a whole lot for us beyond providing a nice installation experience and giving a few Rake tasks.
  • May just be able to remove it.
• At that point, we've basically just rolled our own Webpacker.
  • I'm not totally opposed to this.
  • One extra consideration is that Sprockets and Webpacker also generate GZIP/Brotli variants for production compilation. However, I don't think we're currently using this, so it's basically just a waste of resources anyways. This may change after 18F/identity-devops#3486 .

[zachmargolis]

We could avoid the Sprockets fingerprinting overhead by bypassing Sprockets altogether and dumping the output to public.

I like that the final assets have fingerprints in them, because then it works well when we eventually deploy to a CDN and don't have to deal with asset breakages across deploys, would removing that remove some intermediate step? Or remove fingerprinting entirely?

[aduth]

I like that the final assets have fingerprints in them, because then it works well when we eventually deploy to a CDN and don't have to deal with asset breakages across deploys, would removing that remove some intermediate step? Or remove fingerprinting entirely?

Ah, sorry, I should have clarified: I would still aim to keep the fingerprint, but generate those in Webpack instead, using the [contenthash] template tag. I assume this would also have some impact on performance, though ideally not quite as noticeable as it is currently with Sprockets.

One thing I noticed in perusing Webpacker source is that they include the digest only in production environments, following some Webpack build performance recommendations.

[zachmargolis]

• On this thought and on closer inspection, I'm not really sure that jsbundling-rails does a whole lot for us beyond providing a nice installation experience and giving a few Rake tasks.

  • May just be able to remove it.

I feel like with any of these projects, it's hard to tell what will stay up to date and what will drift and get stale over time. If it looks like jsbundling-rails is up to date for now, we may as well keep it, this PR seems easy enough so if we did want to just remove it and do something more direct, we could do that too? One PR doesn't bind us to a library forever

[aduth]

Yeah, I also forgot it handles the assets:precompile bit, which is nice enough to defer.

[aduth]

Webpacker was better integrated into Rails requests and had some nice quality-of-life features we'd lose:

• Automatic refresh on JavaScript file changes
• Waiting for the Webpack compilation to finish before letting the response be presented

Restored the first of these in 72e1058.

I'm thinking the second could be possible as well, by one of either:

• Do manifest lookup by HTTP request to devserver's version of the manifest, assuming the dev server would hold the request 'til compilation is complete.
• Similar, in manifest lookup, make an HTTP request to a custom dev-server API which implements a long-polling endpoint, released when websocket result indicates compilation is complete.

[aduth]

Build seems anecdotally faster

• Still need to measure

One data point: The assets:precompile phase of the javascript_build CI task takes 32s on this branch, compared to 1m35s on a different, recent branch.

Before: https://app.circleci.com/pipelines/github/18F/identity-idp/64363/workflows/796fe5bb-ffdc-4adb-9a38-1dc55f270cef/jobs/100173
After: https://app.circleci.com/pipelines/github/18F/identity-idp/64455/workflows/f635608a-ac76-45af-a6ce-4755f9e12c3d/jobs/100309

Edit: Also worth reiterating that this opens up the opportunity to switch from Babel to swc-loader. Locally, that reduces time of yarn build by a further 75%, from 8.2s to 2.1s.

[aduth]

• Do manifest lookup by HTTP request to devserver's version of the manifest, assuming the dev server would hold the request 'til compilation is complete.

Solid lead from logging:

[webpack-dev-middleware] wait until bundle finished: /packs/assets-manifest.json

[aduth]

Waiting for the Webpack compilation to finish before letting the response be presented

Actually... this may just work automatically, at least in these sense that:

1. We don't digest generated packs in development
2. Even if Rails were to use a stale manifest, the generated pack URLs are going to be the same
3. Those pack URLs will be referencing the webpack-dev-server URL
4. The webpack-dev-server will wait 'til any pending compilation is complete before serving them up

So even if compilation is in-progress while making the request, there would never be a case where the user would see behavior from the "old" code after refreshing.

One edge-case is if the manifest doesn't yet exist, such as when starting up the server for the first time. In my experience, the build takes less time to become ready than the Rails server. Alternatively, we could omit the manifest file from the clean script so that it continues to exist when starting the server, even if "stale".

Generally, it'd still be a good idea to have some safeguards hereabouts for if the file doesn't exist:

identity-idp/app/helpers/script_helper.rb

Lines 45 to 47 in 54495cc

	def manifest
	@manifest ||= JSON.parse(File.read(MANIFEST_PATH))
	end

[aduth]

Note to self: Do a pass on interoperability with devops workflows, specifically assumptions around Webpacker, public/packs, etc (thinking CloudFront in particular). I see at least a few references in identity-devops to public/packs and public/packs/manifest.json. They may just work as-is, but needs a closer look.

Currently, this branch flattens public/packs and doesn't have a nested js directory. For better compatibility and to anticipate a similar adoption of cssbundling-rails, it may be worth reintroducing it.

[mitchellhenke]

Note to self: Do a pass on interoperability with devops workflows, specifically assumptions around Webpacker, public/packs, etc (thinking CloudFront in particular). I see at least a few references in identity-devops to public/packs and public/packs/manifest.json. They may just work as-is, but needs a closer look.

Currently, this branch flattens public/packs and doesn't have a nested js directory. For better compatibility and to anticipate a similar adoption of cssbundling-rails, it may be worth reintroducing it.

Similar assumptions exist in CircleCI/GitLab CI:

• https://github.com/18F/identity-idp/blob/main/.gitlab-ci.yml#L50-L52
• https://github.com/18F/identity-idp/blob/main/.circleci/config.yml#L178-L180

[aduth]

For better compatibility and to anticipate a similar adoption of cssbundling-rails, it may be worth reintroducing it.

Couple interop improvements here:

• Renamed public/packs/assets-manifest.json back to public/packs/manifest.json, as it was with Webpacker (b88a941)
• Reintroduced nested public/packs/js directory, as it was with Webpacker (afb80bf)

Also confirming that CloudFront/S3 assets are being uploaded correctly during build:

upload: [REDACTED]/public/packs/js/masked-text-toggle-5bf75821.js to s3://[REDACTED]/packs/js/masked-text-toggle-5bf75821.js

[aduth]

Another quality-of-life regression with this is that running feature specs locally will assume that the latest-built JavaScript already exists, whereas previously Webpacker would integrate into the request stack to kick off a compilation if needed. I don't think this is strictly a blocker since it's a matter of running yarn build to resolve, but happy to be overruled.

[mitchellhenke]

Another quality-of-life regression with this is that running feature specs locally will assume that the latest-built JavaScript already exists, whereas previously Webpacker would integrate into the request stack to kick off a compilation if needed. I don't think this is strictly a blocker since it's a matter of running yarn build to resolve, but happy to be overruled.

It would be nice if make test at least compiled?

[aduth]

It would be nice if make test at least compiled?

Yeah, that seems like a reasonable compromise for now 👍 I'll add it.

Edit: See 08bfc38.

[aduth]

It would be nice if make test at least compiled?

Yeah, that seems like a reasonable compromise for now 👍 I'll add it.

Edit: See 08bfc38.

After the fact, I notice that jsbundling-rails will auto-enhance a test:prepare task with the build:

https://github.com/rails/jsbundling-rails/blob/98780f2/lib/tasks/jsbundling/build.rake#L14-L15

I assume that's not automatically invoked, but that someone could run rake test:prepare before running tests? It might be an alternative for what was added in 08bfc38, though at least based how those targets are arranged, I don't feel particularly compelled to change how it's implemented.

[justin808]

Hi @aduth @mitchellhenke @zachmargolis,

@tomdracz, and I have been putting a lot of effort into https://github.com/shakacode/shakapacker, the successor to webpacker.

V6 addressed many of the issues of v5 and prior.

In fact, upon a closer look at the PR description, #5746 (comment), I think shakapacker v6 addresses every one of your concerns.

Since you guys seem to like features like HMR and code-splitting, I'm curious why the transition away from webpacker is helpful?

I took a look at the code and the comments, including this follow-up PR https://github.com/18F/identity-idp/pull/5805/files

It seems that you're reinventing a lot of what is already in webpacker v6, and you'd be a lot better off with shakapacker. We could work together to solve these issues that you're seeing.

And we can be progressive. For example, we've even got SWC.rs support started, added in shakacode/shakapacker#29.

FWIW, I've been working on webpack integration since this article I published in 2014, and I'm the creator of React on Rails.

Anyway, I'm reaching out to see if you'd be interested in working with us on Shakapacker.

[aduth]

Hi @justin808, thanks for reaching out.

You're absolutely right to point out that a lot of what we've implemented here is reinventing the wheel. As mentioned in the original comment, we were beginning to feel held back by Webpacker v5 (Webpack v4) and, absent any alternative known to us at the time, this felt like the most viable path forward.

That being said, our requirements are such that we were already hitting the limits of what we could do within Webpacker. In particular, the code that you reference here has largely existed already from when we were previously using Webpacker, originally introduced in #5223 as a way to allow us a sort of "language pack script" functionality that was only possible by reimplementing parts of the Webpacker manifest script lookup behavior.

I'm glad to see Webpacker continue to live on through Shakapacker. However, speaking for myself, I'm quite happy with the result of the work here and the direct control it allows us over the build configuration. If we start to run into problems with our current approach, we’ll definitely take another look at Shakapacker.

[justin808]

@aduth wrote

we were already hitting the limits of what we could do within Webpacker

Anyway, I'd love to know what Webpacker limits suggest that the extra code you all added to your project is better than Shakapacker v6+.

I think what you wrote applies to older versions of webpacker.

The new v6+ gem:

1. has no version dependencies
2. use webpack-merge to allow easy customization
3. uses a single file webpack.config.js that can be completely custom
4. users a YAML file to keep the view helper in sync with the webpacker configuration
5. has zero requirements that you use the JS webpack part. You can use the view helper, which provides the elegant proxying feature (and caching of the manifest) and other goodness.
6. support for SWC.rs

(@tomdracz, did I miss anything?)

I think you can delete a lot of the boilerplate you added and have a small webpack.config.js that uses webpack-merge and does a few simple customizations of the default and then 💥 you got what you have now and more!

Now that we can quickly iterate on Shakapacker, I think it will be super-amazing super-quickly 🚀 !