refactor(auth)!: Remove generic types #2804
Conversation
Fixes JSON serialization issues and cleans up the API, preventing accidental misuse.
In the past, generic types were necessary since plugin keys returned category wrappers over plugins. The category had to remain generic over any plugin and the plugin key carried the generic types to make the category wrapper statically resemble the plugin. Now that the category and plugin have the exact same interface and the plugin key directly returns the plugin, there is no need for generics.
dnys1
changed the title
dnys1
changed the title
| @@ -16,17 +16,20 @@ import 'package:meta/meta.dart'; | |||
| class CognitoUserAttributeKey extends AuthUserAttributeKey | |||
There was a problem hiding this comment.
why do we have Cognito types in core? I recommend moving the category level behaviors to AuthUserAttributeKey and those that are specific to Cognito out of core and into the Auth package.
There was a problem hiding this comment.
The Cognito configuration (and all plugin configurations) are in core and this is needed as part of it.
category level behaviors to AuthUserAttributeKey
This has already been done unless I missed something?
There was a problem hiding this comment.
this is right, I pulled the PR and looked at the code in details. now I understand the structure and how config is done in core package.
|
|
||
| @zAmplifyGenericSerializable | ||
| class AuthUser<Details extends SignInDetails> | ||
| abstract class AuthUser |
There was a problem hiding this comment.
This is to match iOS/Android
There was a problem hiding this comment.
AuthUser is an interface in iOS and a final class in Android. I think it make sense to not change it to abstract because it is a data class with no behaviour.
There was a problem hiding this comment.
It is a template for a data class given that plugins would extend it and override the SignInDetails type. I'm okay making it instantiable in this case.
| @@ -15,7 +16,11 @@ abstract class AuthUserAttributeKey | |||
| with AWSSerializable<String> | |||
| implements Comparable<AuthUserAttributeKey> { | |||
| /// {@macro amplify_core.auth_user_attribute_key} | |||
| const AuthUserAttributeKey(); | |||
| const factory AuthUserAttributeKey(String key) = _AuthUserAttributeKey; | |||
There was a problem hiding this comment.
I'm not sure I understand the update here. Why add the factory constructor? I see it used in the converter, but you could use _AuthUserAttributeKey directly there, no?
Do we want to expose this for customers?
| const AuthUser({ | ||
| required this.userId, | ||
| required this.username, | ||
| required this.signInDetails, |
There was a problem hiding this comment.
Q: why are we changing this?
There was a problem hiding this comment.
There are tradeoffs to having abstract types in data classes (SignInDetails). Generics are out. If this is included, a fromJson factory cannot be created since SignInDetails has no concrete type to call. I'll add it back to keep the signature the same and remove the fromJson factory.
dnys1
force-pushed
the
chore/next/json-todos
branch
from
553c07d
to
199b416
Compare
Removes generic types from the category/plugin interface and from all data classes. This fixes JSON serialization issues and cleans up the API, preventing accidental misuse of types and making the source code more accessible for first and third-party developers.
In the past, generic types were necessary since plugin keys returned category wrappers over plugins. The category had to remain generic over any plugin and the plugin key carried the generic types to make the category wrapper statically resemble the plugin. Now that the category and plugin have the exact same interface and the plugin key directly returns the plugin, there is no need for generics.