-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Move amplify config into common module #763
Conversation
Codecov Report
@@ Coverage Diff @@
## master #763 +/- ##
==========================================
+ Coverage 87.46% 87.72% +0.26%
==========================================
Files 74 75 +1
Lines 3558 3593 +35
Branches 680 683 +3
==========================================
+ Hits 3112 3152 +40
+ Misses 424 419 -5
Partials 22 22
Continue to review full report at Codecov.
|
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.
Cache is not registered is that ok?
private static _components = []; | ||
private static _config = {}; | ||
|
||
static Auth = null; |
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.
Are any of this being used? I think component should have a category so it can be attached.
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.
Those are just for backward compatibility since they will be assigned in aws-amplify/index.ts
and developers may call
import Amplify from 'aws-amplify';
Amplify.Auth.signIn()
instead of
import { Auth } from 'aws-amplify';
Auth.signIn()
Will add some comments on that.
|
||
static addPluggable(pluggable) { | ||
if (pluggable && pluggable['getCategory'] && typeof pluggable['getCategory'] === 'function') { | ||
this._components.map(comp => { |
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.
Where is checking the category for the component to be the same for the plugin?
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.
It will be checked in each category's addPluggable()
method.
@elorzafe Cache is not registered because for that category we didn't provide such a way to configure it. You need to configure that by calling |
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.
👍 Nice!
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:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.