-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: Add files with Amplify.register to sideEffects array #6867
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #6867 +/- ##
=======================================
Coverage 73.34% 73.34%
=======================================
Files 212 212
Lines 13146 13146
Branches 2470 2566 +96
=======================================
Hits 9642 9642
+ Misses 3340 3310 -30
- Partials 164 194 +30
Continue to review full report at Codecov.
|
https://github.com/aws-amplify/amplify-js-samples-staging/tree/master/samples/next/auth/ssr-auth
|
ericclemmons
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next.js no changes, CRA +455B.
|
Testing a predictions and graphql vue sample I have. Chunks using Chunks using |
ashika01
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
|
@Amplifiyer's sample app with |
sammartinez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🌮
|
@aws-amplify/interactions with Angular prod is working fine too. Computing the diff now... edit: tested on react / vue and they are good 👍 react diff: my chatbot samples seem to have larger diffs, but this maybe something on ui-component/chatbot side (as it already has a large bundle size to start with). Let's see what others get first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good for React Native 👍
No diff bc Metro doesn't support tree shaking.
elorzafe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @manueliglesias ! 🥇
|
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 |
Issue #, if available:
Related to #6844
Description of changes:
This PR adds files that call
Amplify.configure()to the"sideEffects"key inpackage.json.The purpose of this is to hint bundlers to not remove the categories registration when creating the bundle.
Tests were done with an angular 9 application in production mode using
@aws-amplify/authBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.