-
Notifications
You must be signed in to change notification settings - Fork 31
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
[ST-5243] Make splitChunks configurable #167
[ST-5243] Make splitChunks configurable #167
Conversation
@@ -21,6 +21,9 @@ | |||
"utils", | |||
"backpack-addons" | |||
], | |||
"scripts": { | |||
"test:addons": "jest --testPathPattern=backpack-addons" |
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 will also run babelIncludePrefixes.test.js
, the only other test in the folder other than the kitchen sink e2e block. If theres an alternative existing test pattern I'm missing lmk.
}); | ||
|
||
test('should return default if no config defined', () => { | ||
jest.doMock('./test/mockPackage.json', () => ({ |
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 testing setup here was a bit awkward, open to suggestions as always.
Essentially, splitChunks.js
pulls the JSON here:
const paths = require('../config/paths');
const appPackageJson = require(paths.appPackageJson);
const bpkReactScriptsConfig = appPackageJson['backpack-react-scripts'] || {};
So in the test file, we're mocking the result of paths
to be {appPackageJson: './test/mockPackage.json'}
.
The exact path is arbitrary, only matters that we stay consistent.
Then the second line will resolve as const appPackageJson = require('./test/mockPackage.json');
.
So then in every test we can mock out that module using doMock
. We need doMock
instead of .mock
such that the calls aren't hoisted and overriden.
We can't (or at least I can't) use the standard jest mocked module pattern because the module returns a JSON object, not a function.
https://jestjs.io/docs/jest-object#jestdomockmodulename-factory-options
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.
👍
👋
Context
Internal references:
Users of BRS are unable to make full use of the flexibility in code splitting provided by webpack's splitChunks.
In this PR we allow a configuration object to be passed from the consumer's package.json to the BRS webpack config. This is enabled in a similar pattern to the existing vendorsChunkRegex. Passing via JSON does pose some limitations but this feels like a reasonable iteration.
Backwards Compatibility
It's worth first calling out what hasn't changed 🙂
Currently all microsites in Skyscanner use the following config.
This still works as before.
If
enableAutomaticChunking
is true then any new configuration will be ignored and the existing defaults are applied:The
vendorsChunkRegex
will be applied if defined as before.If
enableAutomaticChunking
is false, an empty object is still returned.Note on Webpack 5
Nothing in https://webpack.js.org/migrate/5/ jumps out to me as conflicting. There are references to changes under splitChunks but I don't see them being an issue in any way here. We don't want to complicate the migration so please shout if there are concerns.
Examples
Now the new functionality.
With
enableAutomaticChunking
disabled, the custom configuration objectsplitChunksConfig
will be applied if provided. This allows us to directly inject configuration.Note that we now need to pass the vendors regex in the same manner as any other custom cache group.
We could now also pass any configuration with a primitive value in the same manner if desired, although none of these options appear to be used internally.
MIsc/Qs
Since as mentioned we're going JSON -> JS, we need to find and apply RegExp() to
test
strings. In webpack it's possible to provide functions instead, but with the current approach this isn't easily supported - and currently sees no use within Skyscanner. Most other properties appear to be primitive types so should work without issue.I've added comments, update the existing READMEs and provided examples inline, happy to add further documentation if required.
Open to any suggestions on the testing approach.
Could we publish as a beta and test again locally?