fix: Webpack 5 automatic publicPath error #23760
Conversation
|
Thanks for taking the time to open a PR!
|
|
|
|
@ZachJW34 I know you are familiar with this package (at least more-so than me) and can better evaluate the impact of adding this default value. Mind taking a look? Wondering if this should really be in the batteries-included package? |
|
@emilyrohrbough not too familiar but the change makes sense to me. With the change being in Do you know if we document the usage of our webpack preprocessors with Webpack 5? I feel like a system-test would make sense here, even if it's not officially supported since it seems like a lot of users are doing this. |
|
We do document webpack 5
There have been a few PRs fixing the preprocessor for webpack 5 over the last little while. I agree a system test makes sense, lucky there's a minimal reproduction here we could use: #18435 |
|
@caseyjhol have you tried this change out for a project that contains a dynamic import? I wrote a system test here and had to make some additional changes to disable webpack chunking, otherwise the webpack will create an additional chunk for the dynamic import and then fail to fetch the chunk. |
|
@ZachJW34 Ah, good catch. This change resolved an issue I was having when an import contained a function that called a dynamic import, but that function wasn't actually being called in my spec. I tested calling the function from my spec and ran into the error you described. I think disabling Webpack chunking makes sense for now, but it might be good to get to the root of the problem - why isn't code splitting working? I could imagine a scenario where someone writes a test to explicitly check that code splitting is working as expected. However, I'm sure in most cases having code splitting disabled for tests would be more desirable. |
|
@caseyjhol I don't think there is a valid use case for testing that something was actually code-split as that is an implementation detail of the bundler being used. The bundling of the spec and the bundling of the application under test are separate, so I don't see any value in testing that your specs are being code-split. The As for the PR, do you want me to submit a PR to your fork (given that I have permissions)? Or you could copy/paste the code from my branch and push up a commit. Up to you! |
|
@ZachJW34 Agreed - that makes sense! |
|
Edit: Looks like you got it! Cool will review soon |
|
@caseyjhol looks like the const LimitChunkCountPluginStub = sinon.stub()
webpack.optimize = {
LimitChunkCountPlugin: LimitChunkCountPluginStub,
}to the |
|
|
||
| Module._load = function (request, parent, isMain) { | ||
| if (request === 'webpack' || request.startsWith('webpack/')) { | ||
| const resolvePath = require.resolve(request, { |
There was a problem hiding this comment.
Do we need to try/catch this?
There was a problem hiding this comment.
Wouldn't hurt adding it for safety, where it falls back to the original load if it throws
There was a problem hiding this comment.
Ok, I'll leave it to youif you want to add it... dropped a ✅
|
Nice job @caseyjhol this will be in the next release 🎉 |
User facing changelog
Fixed an issue with Webpack 5 and modules that contain dynamic imports. Fixes #18435.
Additional details
This PR implements the fix suggested in the relevant issue. The default publicPath value in Webpack < 5 is
''. In Webpack 5, it's'auto'. So, this is a backwards compatible change that will have no effect in prior versions of Webpack.PR Tasks
cypress-documentation?type definitions?