-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(aws-amplify): Amplify.register is called per-instance, not per-import #6844
fix(aws-amplify): Amplify.register is called per-instance, not per-import #6844
Conversation
if (JS.isEmpty(config)) return this._config; | ||
if (isEmpty(config)) return this._config; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JS
is @deprecated
, so I fixed it while I was here.
This pull request introduces 1 alert when merging db83aad into 26ce5df - view on LGTM.com new alerts:
|
This only gets ran on iOS, not Web
This pull request introduces 2 alerts when merging dfa2114 into 26ce5df - view on LGTM.com new alerts:
|
import NotificationClass from './PushNotification'; | ||
|
||
const _instance = new NotificationClass(null); | ||
const PushNotification = _instance; | ||
Amplify.register(PushNotification); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still needed though, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is, but this was hoisted into Pushnotification.ts
' constructor
.
Co-authored-by: Alex Hinson <alexmhinson@gmail.com>
Hmmm, I've published to Verdaccio (https://github.com/aws-amplify/amplify-js/blob/main/CONTRIBUTING.md#verdaccio), but now
I may test with https://docs.amplify.aws/start/q/integration/angular instead of https://github.com/aws-amplify/amplify-js-samples-staging/pull/166... |
This pull request introduces 2 alerts when merging 1300192 into 26ce5df - view on LGTM.com new alerts:
|
Changes look good to me, just for my understanding, how does |
@Amplifiyer I ninja-edited the description above:
amplify-js/packages/aws-amplify/src/withSSRContext.ts Lines 34 to 40 in 26ce5df
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, thanks @ericclemmons. This is also for my understanding -- now that we’re registering modules if browserOrNode().isBrowser
is true, how do modules get registered for react native? Still trying to wrap my head around RN flows with Amplify :p
This pull request introduces 1 alert when merging ed5cf13 into 26ce5df - view on LGTM.com new alerts:
|
@wlee221 I'll test RN & Gatsby tomorrow. I'm wondering the same thing 🤔 I forgot our integration tests run on |
ed5cf13
to
5f2e531
Compare
This pull request introduces 1 alert when merging 5f2e531 into 26ce5df - view on LGTM.com new alerts:
|
@amhinson I'll need to change my logic to Otherwise, we would need typeof navigator != 'undefined' && navigator.product == 'ReactNative') |
Going through tests now. Turns out our Jest environment is returning |
Simple steps for fixing Angular amplify bundling behavior:
Proposed steps for "reverting"
|
Going to go with the simpler |
@ericclemmons could you provide which PR to track and the rough bugfix release timeline? We are blocked by #6836 and don't know any workaround fix. |
@mordka We will provide a link to the PR once we have one, we are working on it as we speak to unblock yourself. |
This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs. Looking for a help forum? We recommend joining the Amplify Community Discord server |
Issue #, if available: Fixes #6761, Fixes #6836
Introduced by #6146,
Amplify.register
was moved to per-import (so thatAmplify.Auth
was always the same singleton) rather than per-instance (so that eachreq
creatingnew Auth
on the server wouldn't create memory leaks for the lifetime of the server!).withSSRContext
explicitly registers modules to a newamplify
instance:amplify-js/packages/aws-amplify/src/withSSRContext.ts
Lines 34 to 40 in 26ce5df
This PR reverts:
For validation, I ran production builds of Next.js, CRA, & Angular (https://github.com/aws-amplify/amplify-js-samples-staging/pull/166).
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.