-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Use scratch webpack config #9536
Conversation
BREAKING CHANGE: changes layout of output files and browser compatibility
["@babel/preset-env", {"targets": {"browsers": ["last 3 versions", "Safari >= 8", "iOS >= 8"]}}], | ||
"@babel/preset-env", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to .browserslistrc
"build": "npm run clean && webpack", | ||
"clean": "rimraf ./build && mkdirp build && rimraf ./dist && mkdirp dist", | ||
"clean": "rimraf ./build ./dist", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
webpack makes its own directories, so we don't need to use mkdirp
to do that.
"\\.(jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2|mp4|webm|wav|mp3|m4a|aac|oga)$": "<rootDir>/test/__mocks__/fileMock.js", | ||
"\\.(jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2|mp4|webm|wav|mp3|m4a|aac|oga)\\??$": "<rootDir>/test/__mocks__/fileMock.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were already telling jest
to mock imports for these file types. This tells jest
to keep mocking them even if they include an optional question mark at the end (\??
, but with an extra layer of escaping).
import soundThumbnail from '!base64-loader!./sound-thumbnail.jpg'; | ||
import soundThumbnail from '!base64-loader!./sound-thumbnail.jpg?'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me a bit sad. What I really wanted was a way to say, "hey webpack, please use the new fancy Asset Modules feature with all assets EXCEPT those handled by some other loader." That way, things like import foo from '!base64-loader!./foo.png'
could use base64-loader
and things like import foo from './foo.png'
would use Asset Modules. I tried a few ways to hook that up and couldn't get it working, so instead I went with this: imports with certain file extensions (including jpg
and png
, etc.) are handled as Asset Modules only if they explicitly say so (with a query like ?url
) or have no query at all (not even ?
, an empty query).
import ArgumentType from 'scratch-vm/src/extension-support/argument-type'; | ||
import BlockType from 'scratch-vm/src/extension-support/block-type'; | ||
import {ArgumentType, BlockType} from 'scratch-vm'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reaching into a module to grab some arbitrary script is no longer allowed, so I adjusted scratch-vm
to export these through the main entry point. In the future, I might make these named, non-default exports in scratch-vm
's index.js
(once that's ESM) and/or make them separately-specified entry points in the exports
section of scratch-vm
's package.json
.
global.XMLSerializer = () => ({ | ||
serializeToString | ||
}); | ||
class XMLSerializer { | ||
constructor () { | ||
this.serializeToString = serializeToString; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes an "is not a constructor" error
Buffer: require.resolve('buffer/'), | ||
stream: require.resolve('stream-browserify') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are no longer automatically polyfilled in webpack 5.
webpack.config.js
Outdated
optimization: { | ||
splitChunks: { | ||
chunks: 'all', | ||
name: 'lib.min' | ||
path: path.resolve(__dirname, 'dist') | ||
} | ||
}); | ||
|
||
// build the examples and debugging tools in `build/` | ||
const buildConfig = baseConfig.clone() | ||
.merge({ | ||
devServer: { | ||
client: { | ||
progress: true | ||
}, | ||
runtimeChunk: { | ||
name: 'lib.min' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous version expressed more control over the way webpack would chunk shared code, but letting webpack figure it out leads to more optimal results. Also, using a single output (lib.min
) as the chunk for the webpack runtime and the splitChunks
target and a named entry point (old line 130) is no longer allowed by webpack 5. The new configuration is simpler and, IMO, better.
})); | ||
|
||
// Skip building `dist/` unless explicitly requested | ||
// It roughly doubles build time and isn't needed for `scratch-gui` development |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On my main dev computer, building each target takes a little over 30 seconds, or about 1 minute if you do both (saves like 2-3 seconds).
@@ -1,64 +1,56 @@ | |||
const defaultsDeep = require('lodash.defaultsdeep'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is slightly easier to view if you set "Hide whitespace" (or add &w=1
to the URL)
…-scratch-webpack-config Use scratch webpack config
Proposed Changes
Use our new
scratch-webpack-configuration
to standardize the wayscratch-gui
builds and fix the kinds of problems that are blocking #9533 and other updates.Reason for Changes
General dependency updates, especially in
scratch-svg-renderer
, and especially especially things related tojsdom
, mean we need to change some things about the way we webpack. Some of it's related to webpack 5, but not all of it. I suspect we'll soon need to move to ESM for everything... this set of changes can probably be seen as a precursor to that.I took the opportunity to make further changes. The main one is setting it up so that webpack can build "deeply" -- that is, when webpack builds (for example)
scratch-gui
, it'll access the source code ofscratch-vm
andscratch-svg-renderer
(and more in the future) instead of building their bundled forms. This should give us a more optimal bundle overall. In particular, it will hopefully prevent problems where we're embedding multiple versions of the same Scratch library (likescratch-svg-renderer
) intoscratch-gui
orscratch-www
. That kind of issue was a major component of the "gray question mark issue" from a while back.Some of the reasoning for the particulars of the webpack configuration is available here: https://github.com/scratchfoundation/scratch-webpack-configuration
There's some further cleanup to do, such as moving some of these settings into the shared configuration, but I think this is a good (and working!) time to pause for feedback.
Test Coverage
Affects code already under test. Does not increase or decrease test coverage.
Browser Coverage
Check the OS/browser combinations tested (At least 2)
Coming soon
Mac
Windows
Chromebook
iPad
Android Tablet