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

Minimal Webpack 4 -> 5 implementation [portable] #454

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

towerofnix
Copy link

@towerofnix towerofnix commented Feb 28, 2024

Proposed Changes

This PR follows up #429. Following that PR's code, it does three things:

  • Update dependencies - updates the below, all at once:
    • [dev] webpack@latest (5.90.3, from 4.47.0, both pinned)
    • [dev] webpack-cli@latest (5.1.4, from 3.3.12, both pinned)
    • [dev] terser-webpack-plugin@latest (5.3.10, from 1.4.5, both pinned)
    • file-loader@latest (6.2.0, from 4.3.0, both pinned; same as scratch-vm)
    • worker-loader@latest (3.0.8, from 2.0.0, both un-pinned; retains webpack 4 compat)
  • Removes --colors from webpack scripts, it's no longer needed
  • Updates syntax for usage of worker-loader
    • {"inline":true,"fallback":true} is now {"inline":"fallback"}
  • Remove invalid sourceMap: true option from call to TerserPlugin
    • See Note about source maps in webpack docs. "Works only with source-map, inline-source-map, hidden-source-map and nosources-source-map values for the devtool option. […] Using supported devtool values enable source map generation."
    • Source maps are enabled automatically IF we're using a devtool value that supports it in the first place — which we aren't!
    • scratch-storage (like other Scratch libraries) uses cheap-module-source-map, which Terser doesn't support. See Special cases in webpack docs. "The following options are not ideal for development nor production. They are needed for some special cases, i. e. for some 3rd party tools." However, we decided messing with the source map setting was out of scope for this PR, and only updated the call to TerserPlugin. If a compatible source map setting is used then source maps will automatically be enabled.
  • Change distribution library type for Node.js from commonjs2 to umd
    • In commonjs2 in webpack 5 it's impossible to flat-out replace module.exports with a value of your choosing, e.g. module.exports = "foo bar"; if you do, it will just export as module.exports.ScratchStorage = "foo bar". This is a problem because lots of code depends on importing import ScratchStorage from 'scratch-storage' and would have to be updated to import {ScratchStorage} from 'scratch-storage', which is not ideal. umd2 avoids this problem and the export continues to work as intended.
    • We don't alter the target: 'web' library which was already of type umd2, and so avoids this problem already.
    • I traced this incompatibility back to this commit webpack/webpack@f1dd328 as part of refactor library and allow library per entrypoint webpack/webpack#10448 but couldn't find any existing documentation or other instances of people running into this problem. Seems really weird to me, like it would've been a larger impact incompatibility. Maybe commonjs2 isn't actually a common library type and most people just use umd.
  • No other changes, description above covers everything in this PR to my knowledge.

Reason for Changes

/cc @cwillisf

Minimizing the changes required to port Scratch to Webpack 5, in general, as Webpack 4 is outdated and severely prevents Scratch from being built with anything beyond Node.js 16, which has exited maintenance LTS support as of October 2023, so is eventually or immediately a security issue for Scratch developers (both open-source and in Scratch's own development). See our summary in other pull requests e.g. scratchfoundation/scratch-vm#4171, merged three hours ago as of writing.

This PR takes a "fewest changes necessary" approach. Please note that Webpack 5 includes proper, built-in support for web workers and this PR does not take advantage of it, instead retaining the (deprecated) worker-loader dependency. We do this because the updated version of worker-loader (3.0.8) retains compatibility with Webpack 4. This means scratch-storage supports both webpack 4 and 5, so I describe this PR as "portable".

This PR is technically one degree less minimal than it strictly has to be: It updates the tooling for scratch-storage so that its own development happens with Webpack 5. This is mostly as a proof of concept, i.e. demonstrating the minimal amount of work required to port from 4 to 5, generally speaking. (Obviously, scratch-storage is a much less complex package than scratch-vm or scratch-gui. But still! 👻) But it's also toward the general goal of porting development from 4 to 5 and after this PR scratch-storage no longer needs to be touched, except for niceties and non-portable changes such as using the built-in web worker support rather than a deprecated (albeit back-compatible) module.

scratch-storage is a valuable repository to start with, for a few reasons:

  • It demonstrates and is readily "portable" — this is a small package where a minor change is required to enable compatibility between webpack 4 and 5, so that scratch-storage's development can immediately proceed in webpack 5 (and Node 20, more secure, etc), while other repositories can update to webpack 5 later. This is valuable from an NPM hijinks standpoint — there's no need to worry about updating scratch-storage at the same time as other repositories, for example. It may be possible for other, larger packages (like scratch-vm) to be updated similarly portably, but this PR shows off the concept in a nice, bite-sized manner.
  • Other packages cannot be updated to webpack 5 until this is merged, i.e. this blocks progress in scratch-render, scratch-vm, etc. This is because scratch-storage currently isn't compatible with webpack 5 - that's exactly what this PR seeks to resolve, and no more (from a dependant's standpoint).
  • Again on the simplicity count, this just shows how updating from 4 to 5 really isn't that big of a deal. There should be a focus on backwards compatibility with Webpack 4 in individual packages where possible, because the more "isolated" each step is, the less we have to panic over orchestrating more involved changes across multiple repositories at a time. (It would be great if updating scratch-www to Webpack 5 were the final boss — rather than updating a whole slew of repositories at once, including scratch-www!)

Edit: Oh hey, scratch-www already moved to Webpack 5 while I wasn't looking ^_^ 🥳

All the same this helps get closer to a scratch-vm and scratch-gui that's built and developed in Webpack 5!

Test Coverage

Tested in various ways:

  • Build works, tests continue to pass
  • Linked with scratch-vm, tests continue to pass
  • Linked with scratch-gui, playground continues to function
    • This is the only way I actually know of to confirm that scratch-storage is working "in context", since scratch-gui internally imports it.

Note: When testing locally, I do get certain warnings about downloading source maps for what appears to be a web worker, which I thought might be related (see the notes on source maps above):

Cannot load blob://nullhttp//localhost:8601/f6240eab828e6d415177.worker.js.map due to access control checks.

However, these appear when I test with the unmodified/npm published scratch-storage too, so they don't appear to be introduced by this pull request.

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

1 participant