refactor(storage): decouple utils from Amplify singleton

[israx]

Description of changes

The following changes decouple internal Storage utils and the uploadData API from the Amplify singleton. This refactor will allow the high-level APIs, the ones exposed to customers, to manage and create a Storage configuration based on an API input or Amplify configuration. Then the obtained config will be accessed by the low level implementation functionalities, e.g putObjectJob, getMultipartUploadHandlers, and resolveS3ConfigAndInput utils.

Out of the scope

• PR doesn't refactor the functionality of the internal Storage utils
• Decouple additional Storage APIs other than uploadData

Arch Diagram

All of the storage APIs will follow this pattern

Issue #, if available

Description of how you validated changes

Checklist

• PR description included
• yarn test passes
• Unit Tests are changed or added
• Relevant documentation is changed or added (and PR referenced)

Checklist for repo maintainers

• Verify E2E tests for existing workflows are working as expected or add E2E tests for newly added workflows
• New source file paths included in this PR have been added to CODEOWNERS, if appropriate

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[israx]

This function is responsible for creating the storage configuration used by the internal storage functionalities. In a subsequent PR, I will expand the functionality of this method to set the Storage config based on an API input or Amplify config.

[israx]

This is how the other Storage APIs will be refactored

[ashika112]

so basically config is decided based on API input or Amplify before calling in the internalUpload, right?!

[israx]

yeah correct

[AllanZhengYP]

The change makes sense to me given the the direction of following PRs: https://github.com/aws-amplify/amplify-js/pull/13562/files#r1671211697

The new createStorageConfiguration serves as a place holder to eventually take responsbility of most of the options resolution and assertion.

[ashika112]

so basically config is decided based on API input or Amplify before calling in the internalUpload, right?!

[jimblanc]

Looks pretty good. I have a few nits around naming and organization, but nothing functional.

[jimblanc]

All the other internal APIs take an instance of the Amplify class, should this new one do the same for consistency until we can refactor everything? If this is just setting the pattern and the other APIs will be refactored in this feature branch, I'm okay with it for now.

[israx]

I'll be refactoring the other internal APIs to adhere to this pattern

[jimblanc]

IMO we should still call this configuration to keep naming consistent within the library (like we do with library options). Also, what's the harm of just using StorageConfig directly? We already have a single point where the options get parsed out (i.e. resolveS3ConfigAndInput).

[israx]

Calling it Config will conflict with the internal interface names. The intention is to have a common interface that is separate from the Amplify Config and doesn't make any reference to it.

[jimblanc]

But it literally just contains the configuration, where is the conflict coming from? I think having the separate interface makes sense for the purpose S3Configuration is serving but around your point of having an interface that doesn't make any reference to Amplify configuration, isn't that what this Alias type is doing?

[israx]

I'm leaving the S3InternalConfig for reference

interface S3InternalConfig {
	serviceOptions: S3ServiceOptions;
	libraryOptions: S3LibraryOptions;
	credentialsProvider(): Promise<AWSCredentials>;
	identityIdProvider(): Promise<string>;
}

So I'd like to avoid changing the S3ServiceOptions alias for the following reasons:

1. It is internal only and it is not exported
2. It will require the S3InternalConfig to adhere to the new name as well. For instance, serviceOptions need to be changed to config or confing<something>. So the internal APIs refereing to this name will end up looking,

const resolveS3ConfigAndInput ({config}:ResolveS3ConfigAndInputParams){
config.config
}

[jimblanc]

Ah okay got it. Repeating from another comment but: I think where I'm getting hung up is that something like a credentialProvider is not configuration as per the idioms we've defined in the library. I'm okay moving forward with this to unblock things, but maybe we can consider more when we refactor the other APIs. Regarding you example of resolveS3ConfigAndInput, maybe that's a sign we should just rename the function 🤔

[jimblanc]

Could this just extend or reference S3Configuration to avoid the duplication? For example, we could do something like:

await resolveS3ConfigAndInput({
  s3Config: configuration,
  apiOptions: downloadDataOptions,
});

To keep the separation clean instead of spreading.

[israx]

I think the s3Config could be called config only. Otherwise it will conflict with another key that is called s3Config too