feat(core): update storage outputs class for multi-bucket support #5476
Conversation
| @@ -23,6 +23,9 @@ class StorageOutputs | |||
| /// The Amazon S3 bucket name. | |||
| final String bucketName; | |||
|
|
|||
| /// The list of buckets if there are multiple buckets for the project | |||
| final List<Map<String,String>>? buckets; | |||
There was a problem hiding this comment.
I suggest to define a class Bucket and use it instead of Map.
| /// {@template amplify_core.amplify_outputs.amazon_pinpoint_channel} | ||
| /// Supported channels for Amazon Pinpoint. | ||
| /// {@endtemplate} | ||
| class StorageOutputBucket { |
There was a problem hiding this comment.
| class StorageOutputBucket { | |
| class BucketOutputs { |
| factory StorageOutputBucket.fromJson(Map<String, dynamic> json) => StorageOutputBucket(json['name'].toString(), json['bucket_name'].toString(), json['aws_region'].toString()); | ||
| String name; | ||
| String bucketName; | ||
| String awsRegion; | ||
| Map<String, dynamic> toJson() => <String, dynamic>{'name':name, 'bucket_name':bucketName, 'aws_region':awsRegion}; | ||
| } |
There was a problem hiding this comment.
instead of wrting the fromJson and toJson use generated code similar to other Amplify Outputs types.
There was a problem hiding this comment.
if these functions are not written into the class no code gets generated and an error is thrown because the generator does not know how to serialize and deserialize this object
| /// Supported channels for Amazon Pinpoint. | ||
| /// {@endtemplate} | ||
| class StorageOutputBucket { | ||
| StorageOutputBucket(this.name, this.bucketName, this.awsRegion); |
There was a problem hiding this comment.
the ctor can be const. I would suggest to use named parameter similat to other Amplify Outputs type
There was a problem hiding this comment.
I dont know how this got changed, I think I clicked a vscode 'optimization' by mistake, I will change this back right now
| part 'bucket_output.g.dart'; | ||
|
|
||
| @zAmplifyOutputsSerializable | ||
| class BucketOutput |
There was a problem hiding this comment.
| class BucketOutput | |
| class BucketOutputs |
There was a problem hiding this comment.
missing doc comments https://dart.dev/effective-dart/documentation#doc-comments.
NikaHsn
changed the title
There was a problem hiding this comment.
Question: I see the test data has been updated, but are the tests written in a way that validate the new functionality too? Do they need to be updated?
There was a problem hiding this comment.
This is a good question, since multiple buckets are optional, I think we should validate that it works when only one bucket is supplied too. I'll check this out
There was a problem hiding this comment.
I've verified that the test does work properly when only a single bucket is supplied and that the test itself actually validates the new function for multi-bucket support
added the list names buckets in storage_outputs.dart, built the new generator file for it, and updated the test to match the expected json output as described in the api review doc