fix: #4311 Update main entry field to point to CJS builds instead of … #4678
Conversation
…instead of webpack bundles
Codecov Report
@@ Coverage Diff @@
## master #4678 +/- ##
=======================================
Coverage 76.29% 76.29%
=======================================
Files 174 174
Lines 9628 9628
Branches 1965 1965
=======================================
Hits 7346 7346
Misses 2137 2137
Partials 145 145Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Trying to figure out how to best validate it:
• Jest's auto-mocking bug (#4499)
• Confirm that a production build of CRA with latest vs. #4678 yields the same size.
• Since the ./index.js entry-point is no longer referenced, does that mean we lose the process.env.NODE_ENV === "production") behavior?
The way I've validated package-publishing changes like these in the past was by publishing 0.0.0-f2546c6 (or similar) for consumption in stand-alone projects.
How would you recommend validation? The usual yarn build/link?
Jest's auto-mocking bug (#4499)Verified locally that this PR fixes the mocking issue with @aws-amplify/auth in #4499 Confirm that a production build of CRA with latest vs. #4678 yields the same size.CRA app with latest amplify version 500.75 KB build/static/js/2.0357ba12.chunk.js
3.65 KB build/static/css/2.a1f9020b.chunk.css
1.1 KB build/static/js/main.4d3758ea.chunk.js
762 B build/static/js/runtime~main.a8a9905a.js
523 B build/static/css/main.f5532a38.chunk.cssCRA app with this PR changes 500.82 KB (+63 B) build/static/js/2.aae62737.chunk.js
3.65 KB build/static/css/2.a1f9020b.chunk.css
1.09 KB (-2 B) build/static/js/main.d877e69f.chunk.js
762 B build/static/js/runtime~main.a8a9905a.js
523 B build/static/css/main.f5532a38.chunk.cssSince the ./index.js entry-point is no longer referenced, does that mean we lose the process.env.NODE_ENV === "production") behavior?Yes, it'll only be applicable to users who are explicitly depending on the root index.js. If we feel it's no longer important we can even delete the index.js and figure out a better way to publish the webpack bundles. |
There was a problem hiding this comment.
@Amplifiyer Thanks for the validation, and great work on getting this put together so quickly! 🎉
As far as I know, the value in process.env.NODE_ENV is for gating dev-only behavior (e.g. React's warnings and stricter checks) when not in production, and for clearer stack-traces when a bug occurs within our source.
For now, I believe this PR is a step forward for our customers and we can address the bundling and we can handle these quality-of-life separately.
This fix reverts our environment checks to a previous version that worked fine, but failed under certain curcustances now fixed (see aws-amplify#4678) The solution using `new Function'return this')()` caused problems when using CSP
* fix(@aws-amplify/core): Revert environment checks This fix reverts our environment checks to a previous version that worked fine, but failed under certain curcustances now fixed (see #4678) The solution using `new Function'return this')()` caused problems when using CSP * Make node tests use cjs instead of umd
* fix(@aws-amplify/core): Revert environment checks This fix reverts our environment checks to a previous version that worked fine, but failed under certain curcustances now fixed (see aws-amplify#4678) The solution using `new Function'return this')()` caused problems when using CSP * Make node tests use cjs instead of umd
|
This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs. Looking for a help forum? We recommend joining the Amplify Community Discord server |
…webpack bundles
Issue #, if available: fixes: #4311 #4499
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.