Skip to content
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

Update non-UMD build configs to not include source map files #546

Merged
merged 4 commits into from
Jan 25, 2023

Conversation

suniahk
Copy link
Contributor

@suniahk suniahk commented Dec 8, 2022

Many issues have cropped up since Create-react-app 5's release regarding console warnings about source maps. Ultimately, the issue comes down to not including the src directory with the npm bundle. While doing so would clean up the issues, that leads to problems where a developer trying to debug their own use of this library may make source changes without updating their build files, leading to more frustration. Other proposed solutions also have their own issues:

  • Issue with node_modules #512 (comment) suggests GENERATE_SOURCEMAP=false, which disables sourcemaps for everything within a project. This is absolutely overkill for a single package and should be seen as a last resort, not a solution.
  • @zxing throwing source maps warnings for webpack5  SAP/ui5-webcomponents#5648 In many cases, this SAP issue has been mentioned suggesting that the current accepted solution is to simply use the UMD version of the build output. While this can work, this basically kicks the proverbial can down the road in that not all projects are able to/willing to use UMD modules without additional time and effort.

This PR adds lines to each individual build config file to prevent the generation of the source map files. By doing so, source map generation should stay enabled for development (such as it is), while any new builds should avoid the assault of console warnings.

@suniahk
Copy link
Contributor Author

suniahk commented Dec 8, 2022

As a side note, this also resolves #443.

@lnhrdt
Copy link

lnhrdt commented Jan 24, 2023

@odahcam & @werthdavid I know the project is in maintenance mode, but I think this might deserve to get addressed as maintenance. The amount of warnings generated by this library in some popular configurations make a projects warnings in the terminal unusable. The only workarounds I've found myself or mentioned by others involve suppressing these warnings in a way that disables some checks in an entire project, not just this library.

@suniahk mentioned this should resolve #443, but I believe this will also address #499 and #512.

@werthdavid
Copy link
Member

I'll take a look at the PR! Thanks it is much appreciated. While in general I'm not doing much besides updating the library to be compatible Im happy to merge PRs and create a release. Also of the PR has new functionality it is more than welcome, it's just that this whole thing got really big and I don't really have time to make changes on my own.

@werthdavid werthdavid merged commit e8ba123 into zxing-js:master Jan 25, 2023
@werthdavid
Copy link
Member

Will create a release later today thanks @suniahk

@lnhrdt
Copy link

lnhrdt commented Jan 25, 2023

@werthdavid thanks for jumping in to look at the PR and merge.

I'll help test and provide feedback, but it looks like v0.19.2 hasn't made it to to the npm registry yet.
https://www.npmjs.com/package/@zxing/library?activeTab=versions
https://github.com/zxing-js/library/actions/runs/4003861518

@werthdavid
Copy link
Member

that's right. I need to fix CI first...

@werthdavid
Copy link
Member

0.19.2 should be available now

@IrregularShed
Copy link

I'm glad I spotted this, as the change has broken my build 🤦

Rolling back to 0.18.6 for the time being. My client doesn't actually need the functionality ZXing.js was providing any more, so I'll pick things apart to remove the library once I've got a temporary build for them, rather than worry about trying to reimplement.

@lnhrdt
Copy link

lnhrdt commented Jan 25, 2023

@werthdavid 0.19.2 works great for me.

I tested it in both a Webpack 5 (Create React App v5) project and a Webpack 4 (Create React App v4) project. In the Webpack 5 project the console warnings about source maps were resolved. In both projects functionality remained the same and TypeScript definitions continued to work just fine.

Thank you @suniahk for helping to address this issue and @werthdavid for getting it merged & released!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants