fix: errors generated by source maps loader #428
Conversation
|
Thanks for the pull request, @solababs! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
openedx-webhooks
added
the
open-source-contribution
solababs
changed the title
solababs
changed the title
|
Hey @solababs, thank you for this contribution! We'll get tests enabled for you next. |
itsjeyd
added
the
needs test run
There was a problem hiding this comment.
👍
The change looks good in general, but I think it's misplaced.
Additionally, what are the potential downsides of this? We are suppressing a warning, can we alternatively generate this sourcemap ourselves if the package doesn't provide it?
- I tested this: tested againt building an MFE.
- I read through the code
- [na] I checked for accessibility issues
- [na] Includes documentation
lib/createConfig.js
Outdated
| }, | ||
| ], | ||
| }; | ||
| return merge(baseConfig, {...ignoreWarnings, ...configFragment}); |
There was a problem hiding this comment.
I'm not sure, this is the right place for this. I think you should move it to config/webpack.common.config.js or config/webpack.prod.config.js.
There was a problem hiding this comment.
@xitij2000 thanks for the review
what are the potential downsides of this?
we would not be able to see source maps for vendor packages that have mis-configured source-maps that are in the node_modules
We are suppressing a warning, can we alternatively generate this sourcemap ourselves if the package doesn't provide it?
I tried generating the source-maps using rules but still got the same errors, best guess is some mis-configuration on some of the packages.
Of course we can move it to the webpack config files.
I'll update that
There was a problem hiding this comment.
Instead of duplicating this, won't moving this to the common file fix the need for duplication.
e0d
removed
the
needs test run
|
Hi @solababs , looks like the same issue with the commit lint checker here too. Can you amend the messages to follow our standard? |
solababs
force-pushed
the
solababs/fix-source-maps-errors
branch
from
615b33d
to
4d11120
Compare
|
@xitij2000 Just checking in to see if you're good with these changes now? @abdullahwaheed An early heads-up that this PR should be ready for engineering review by fed-bom soon. CC @solababs |
|
|
🎉 This PR is included in version 12.9.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
This PR fixes errors generated by source-maps-loader for some packages.
Careful consideration is taken to only ignore issues generated by node modules as some third party packages may ship miss-configured sourcemaps, that interrupts the build.
See: facebook/create-react-app#11278 (comment) and facebook/create-react-app#11752
Supporting information
This PR fixes openedx/paragon#1674
Testing instructions
On running
npm run buildon any of the MFE's you would have an error similar to the belowThe error to look out for here is
Failed to parse source mapAfter this fix the error should be gone and not show up on running
npm run build.Some MFE's have multiple of these errors and should be gone also.
Fixes: openedx/paragon#1674
Private-ref: https://tasks.opencraft.com/browse/BB-7705