fix: 1.7.3 to 1.7.4 regression __nccwpck_require__ is not defined #195
Conversation
Remove the use of the deprecated Webpack MainTemplate. If `__nccwpck__` is not present, fall back to `__webpack_require__`. It fixes the regression from 1.7.3 to 1.7.4 vercel#188 and it also allows for compatibility with `rspack` https://github.com/web-infra-dev/rspack.
|
Let's revert yarn.lock since that isn't strictly necessary to fix the regression right? All you need is the regex replace. |
|
Would it be possible to update webpack to v5.99.9 before this? I had the text fixtures updated for that version. If we eventually have to update webpack, the corresponding work is already done here. |
ran and updated all tests
|
They should be separate. A regression fix should be shipped as a patch and webpack update can be minor. I think that was the mistake in the previous release when the regression was introduced. |
|
Nevermind. I reapplied the changes with webpack v5.76.0 to keep things simple. The coverage outputs are generally no longer divided into two categories. That's only applicable for v5.99.9. However, I kept the commits intact while patching the files, so don't let that cause any confusion. |
| const fs = require('fs'); | ||
|
|
||
| expect(output.length).toBe(16); | ||
| expect([16, 17]).toContain(output.length); |
There was a problem hiding this comment.
Can you explain why this is no longer 16?
There was a problem hiding this comment.
The explanation is in the second half of the description in #194. In short, running the build within the test many times, webpack sometimes outputs lines with "assets by status" on top of "assets by path". Allowing an extra line would avoid unexpected test failures.
There was a problem hiding this comment.
Why “sometimes”? I would expect the output to be deterministic.
There was a problem hiding this comment.
I don't know what exactly caused this. The build config isn't changed. The output files and bytes appeared to be similar, but I got different outputs by running the test 13 times for this attempt.
2025-07-03_19-49-09.mp4
There was a problem hiding this comment.
Ok we can keep this for now and see if upgrading webpack fixes it in a future PR
| /******/ /* webpack/runtime/asset-relocator-loader */ | ||
| /******/ (() => { | ||
| /******/ if (typeof __webpack_require__ !== 'undefined') __webpack_require__.ab = __dirname + "/"; | ||
| /******/ })(); |
There was a problem hiding this comment.
Is the IIFE required? Perhaps we can make this diff much smaller if we kept the output similar so there is no IIFE wrapper.
There was a problem hiding this comment.
I don't think the IIFE is necessary. The isolation is turned on by default with RuntimeModule, but we can override the method to turn it off. Another patch is incoming.
There was a problem hiding this comment.
The changes are available in the latest commit 0fe6976
styfle
left a comment
styfle
left a comment
There was a problem hiding this comment.
Looks good, just need to fix the coverage
|
Codecov deprecated their language-specific uploaders https://about.codecov.io/blog/codecov-uploader-deprecation-plan/. This might need to be fixed and tested from your end. |
styfle
changed the title
|
🎉 This PR is included in version 1.7.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
While exploring the idea of dropping ncc and using webpack to build the loader directly in this branch, I noticed that 31 outputs differ from their corresponding coverage outputs. These differences are consistent across the changes made here, so I believe we can update the coverage outputs to reflect this variation.