-
Notifications
You must be signed in to change notification settings - Fork 199
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
feat(Core): bootstrap Auth configuration before other categories, and fixed analytics integration tests #475
Conversation
AmplifyPlugins/Analytics/AWSPinpointAnalyticsPluginIntegrationTests/README.md
Outdated
Show resolved
Hide resolved
AmplifyPlugins/Analytics/AWSPinpointAnalyticsPluginIntegrationTests/README.md
Outdated
Show resolved
Hide resolved
AmplifyPlugins/Analytics/AWSPinpointAnalyticsPluginIntegrationTests/README.md
Outdated
Show resolved
Hide resolved
can you change the name of this PR to something like we are trying to follow conventional commits btw (https://www.conventionalcommits.org/en/v1.0.0/) |
this is a fairly important change, it would be good to update the description to talk about the change to configuring Auth first, or very least mention it |
AmplifyPlugins/Analytics/HostApp/Configuration/amplifyconfiguration.json
Outdated
Show resolved
Hide resolved
…nore configuration file in gitignore
e4f043a
to
f286d6f
Compare
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.
Please verify that removing the file AmplifyPlugins/Analytics/HostApp/Configuration/amplifyconfiguration.json
still allows all of the frameworks, both library and test, to compile.
@@ -101,23 +101,24 @@ extension Amplify { | |||
try configure(Logging, using: configuration) | |||
try configure(Hub, using: configuration) | |||
|
|||
// Auth is a special case for other plugins which depend on using Auth when being configured themselves. | |||
try configure(Auth, using: configuration) |
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.
Let's set up an array of Categories to be initialized, in order, then we can inspect membership in the array rather than multiple not-equals checks below. We can aid this with an extension on CategoryType:
In CategoryType.swift
public extension CategoryType {
var category: Category {
switch self {
case .analytics:
return Amplify.Analytics
case .api:
return Amplify.API
case .auth:
return Amplify.Auth
case .dataStore:
return Amplify.DataStore
case .hub:
return Amplify.Hub
case .logging:
return Amplify.Logging
case .predictions:
return Amplify.Predictions
case .storage:
return Amplify.Storage
}
}
}
Then here in AmplifyConfiguration.swift
:
let manuallyConfiguredCategories = [CategoryType.logging, .hub, .auth]
for categoryType in manuallyConfiguredCategories {
try configure(categoryType.category, using: configuration)
}
let remainingCategories = CategoryType.allCases.filter { !manuallyConfiguredCategories.contains($0) }
...
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.