feature(multi-bucket): add multi-bucket support to storage components #5562
Conversation
🦋 Changeset detectedLatest commit: 23e7619 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
jordanvn
changed the title
jordanvn
changed the title
| @@ -12,6 +12,15 @@ import { | |||
| } from './ui'; | |||
| import { StorageManagerDisplayText, PathCallback, UploadTask } from './utils'; | |||
|
|
|||
| interface BucketInfo { | |||
There was a problem hiding this comment.
Nit: should we share this type between components?
There was a problem hiding this comment.
Yes, that makes sense to me. It didn't seem substantial enough to create a new shared types file for storage components, so I went with the definition in the Storage Manager types. There are already exported types and interfaces in that file, and none aside from props in Storage Image types.
There was a problem hiding this comment.
Can we just import and extend FileUploaderProps in the assignment of StorageManagerProps and dedupe this file?
There was a problem hiding this comment.
Are you suggesting the removal of this file and providing its exports from FileUploader types? Or reducing StorageManagerProps to export interface StorageManagerProps extends FileUploaderProps {}?
There was a problem hiding this comment.
Keeping this file and doing the latter
There was a problem hiding this comment.
Got it. Thanks for the clarification
There was a problem hiding this comment.
Updated file to extend FileUploaderProps instead of duplicating type declaration
| <FileUploader | ||
| acceptedFileTypes={['image/*']} | ||
| bucket={{ | ||
| bucketName: 'my-bucket-96e87892835c413e9963f3004a44e1ff', |
There was a problem hiding this comment.
Note: this is not an actual bucket name
There was a problem hiding this comment.
Can you add a note so we don't wonder about that later, or make it more obvious like "my-bucket-xxxxxxx"
There was a problem hiding this comment.
Yep, I like going with "my-bucket-xxxxxxxx"
| @@ -170,6 +170,27 @@ describe('StorageImage', () => { | |||
| ); | |||
| }); | |||
|
|
|||
| it('should pass bucket to getUrl when supplied', () => { | |||
There was a problem hiding this comment.
Note: we're not duplicating the testing of the functionality of the API supporting this, so this test is just confirming that the "bucket" property is successfully passed from the component to the API in the expected format.
| /** | ||
| * Designates the bucket to upload to, if the user has multiple buckets configured | ||
| */ | ||
| bucket?: StorageBucket; |
There was a problem hiding this comment.
Same comment as the one on StorageManager, not sure this is needed since multi bucket support doesn't exist in Gen1?
Description of changes
Changes:
<StorageImage />and<StorageManager />.StorageBucketobject containing the bucket name generated on S3 and the region it exists inIssue #, if available
N/A
Description of how you validated changes
Checklist
yarn testpasses and tests are updated/addeddocs,e2e,examples, or other private packages.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.