Skip to content

Conversation

jimblanc
Copy link
Contributor

@jimblanc jimblanc commented Nov 7, 2022

Issue #, if available:

Description of changes:
This change cleans up lingering usage of deprecated default exports from the amplify-js repo, which will be fully removed for the upcoming v5 release.

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

@jimblanc jimblanc requested review from a team as code owners November 7, 2022 19:53
Comment on lines +17 to 21
import { Amplify } from 'aws-amplify'
import * as AmplifyModules from 'aws-amplify'
import { AmplifyPlugin } from 'aws-amplify-vue'
import awsconfig from './aws-exports'
Amplify.configure(awsconfig)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit

Suggested change
import { Amplify } from 'aws-amplify'
import * as AmplifyModules from 'aws-amplify'
import { AmplifyPlugin } from 'aws-amplify-vue'
import awsconfig from './aws-exports'
Amplify.configure(awsconfig)
import * as AmplifyModules from 'aws-amplify'
import { AmplifyPlugin } from 'aws-amplify-vue'
import awsconfig from './aws-exports'
AmplifyModules.Amplify.configure(awsconfig)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I went back and forth on this one, seemed cleaner to just have two imports (to have the standard Amplify.configure call) but willing to change if you have strong opinions about it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need both? what does the * as AmplifyModules accomplish here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So with default exports you're able to import the default and also star import the rest of the module, e.g. the original code: import Amplify, * as AmplifyModules from 'aws-amplify'

This really isn't possible with named exports, so something like this is not valid for example: import { Amplify }, * as AmplifyModules from 'aws-amplify'. We can do what Chris suggested, but I personally thought having AmplifyModules.Amplify.configure(awsconfig) felt weirder than just have two imports so we can use the standard Amplify.configure form which is used everywhere else. Can change though if folks feel strongly about it.

Copy link
Contributor

@abdallahshaban557 abdallahshaban557 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left a question

Comment on lines +17 to 21
import { Amplify } from 'aws-amplify'
import * as AmplifyModules from 'aws-amplify'
import { AmplifyPlugin } from 'aws-amplify-vue'
import awsconfig from './aws-exports'
Amplify.configure(awsconfig)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need both? what does the * as AmplifyModules accomplish here?

Copy link
Contributor

@abdallahshaban557 abdallahshaban557 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@tannerabread tannerabread left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jimblanc jimblanc merged commit 620b36d into aws-amplify:main Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants