fix(aws-amplify): Amplify.register is called per-instance, not per-import #6844

ericclemmons · 2020-09-21T18:42:10Z

Issue #, if available: Fixes #6761, Fixes #6836

Introduced by #6146, Amplify.register was moved to per-import (so that Amplify.Auth was always the same singleton) rather than per-instance (so that each req creating new Auth on the server wouldn't create memory leaks for the lifetime of the server!).

withSSRContext explicitly registers modules to a new amplify instance:

amplify-js/packages/aws-amplify/src/withSSRContext.ts

Lines 34 to 40 in 26ce5df

    
           		amplify.register(new m.constructor()); 
        
           	} 
        
           }); 
        
           // Associate new module instances with this amplify 
        
           modules.forEach(m => { 
        
           	amplify.register(new m.constructor());

This PR reverts:

167c787
(Partially) fd830eb

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.

This reverts commit 167c787.

packages/pushnotification/src/PushNotification.ts

ericclemmons · 2020-09-21T18:44:45Z

packages/pushnotification/src/PushNotification.ts

-		if (JS.isEmpty(config)) return this._config;
+		if (isEmpty(config)) return this._config;


JS is @deprecated, so I fixed it while I was here.

lgtm-com · 2020-09-21T18:49:09Z

This pull request introduces 1 alert when merging db83aad into 26ce5df - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

This only gets ran on iOS, not Web

lgtm-com · 2020-09-21T19:52:03Z

This pull request introduces 2 alerts when merging dfa2114 into 26ce5df - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

amhinson · 2020-09-21T20:39:43Z

packages/pushnotification/src/index.ts


 const _instance = new NotificationClass(null);
 const PushNotification = _instance;
-Amplify.register(PushNotification);


This is still needed though, right?

It is, but this was hoisted into Pushnotification.ts' constructor.

packages/pushnotification/__tests__/PushNotification-test.ts

Co-authored-by: Alex Hinson <[email protected]>

ericclemmons · 2020-09-21T21:28:48Z

Hmmm, I've published to Verdaccio (https://github.com/aws-amplify/amplify-js/blob/main/CONTRIBUTING.md#verdaccio), but now ng build --prod fails with this:

❯ yarn build
yarn run v1.22.4
$ ng build

ERROR in node_modules/@aws-amplify/ui-angular/dist/amplify-module.d.ts:1:22 - error NG6002: Appears in the NgModule.imports of AppModule, but could not be resolved to an NgModule class.

This likely means that the library (@aws-amplify/ui-angular) which declares AmplifyUIAngularModule has not been processed correctly by ngcc, or is not compatible with Angular Ivy. Check if a newer version of the library is available, and update if so. Also consider checking with the library's authors to see if the library is expected to be compatible with Ivy.

1 export declare class AmplifyUIAngularModule {
                       ~~~~~~~~~~~~~~~~~~~~~~

error Command failed with exit code 1.

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...

lgtm-com · 2020-09-21T21:32:46Z

This pull request introduces 2 alerts when merging 1300192 into 26ce5df - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

Amplifiyer · 2020-09-21T21:33:24Z

Changes look good to me, just for my understanding, how does Amplify.register() gets called in server/node context?

ericclemmons · 2020-09-21T21:46:55Z

@Amplifiyer I ninja-edited the description above:

withSSRContext explicitly registers modules to a new amplify instance:

amplify-js/packages/aws-amplify/src/withSSRContext.ts

Lines 34 to 40 in 26ce5df

    
           		amplify.register(new m.constructor()); 
        
           	} 
        
           }); 
        
           // Associate new module instances with this amplify 
        
           modules.forEach(m => { 
        
           	amplify.register(new m.constructor());

wlee221

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

lgtm-com · 2020-09-21T22:52:08Z

This pull request introduces 1 alert when merging ed5cf13 into 26ce5df - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

ericclemmons · 2020-09-22T00:32:02Z

@wlee221 I'll test RN & Gatsby tomorrow.

I'm wondering the same thing 🤔

I forgot our integration tests run on main, not in PRs, so I'm not comfortable merging and potentially getting surprised!

lgtm-com · 2020-09-22T00:48:21Z

This pull request introduces 1 alert when merging 5f2e531 into 26ce5df - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

ericclemmons · 2020-09-22T17:11:45Z

@amhinson I'll need to change my logic to if (!browserOrNode().isNode). In React Native, browserOrNode returns:

Otherwise, we would need if (isBrowser || isReactNative) where isReactNative comes from:

typeof navigator != 'undefined' && navigator.product == 'ReactNative')

ericclemmons · 2020-09-22T18:48:59Z

Going through tests now. Turns out our Jest environment is returning { isBrowser: true, isNode: true } 🤦

ericclemmons · 2020-09-23T21:56:05Z

Simple steps for fixing Angular amplify bundling behavior:

Explicitly list sideEffects to where Amplify.register happens:

 "sideEffects": [
     "dist/aws-amplify-auth.js",
     "lib/Auth.js",
     "lib-esm/Auth.js",
     "src/Auth.ts"
   ]

Test yarn build --prod output of CRA, Next.js, Vue, & Angular for bundle-size differences.

Proposed steps for "reverting" `Amplify.register` behavior:

Remove all if (...) checks around Amplify.register.
Audit what references are added to Amplify._components. In my testing, I saw this being a combination of categories, [Function], and undefined! This may be because some categories lack getModuleName, but I couldn't track it down.
Ensure that Amplify.register(category) overrides Amplify[category] so that there is only a single reference.
On the client (browser & React Native), console.warn if Amplify.register is called again for a pre-registered category.

We don't want this behavior on the server, because it's expected that new instances of Auth will be created and registered repeatedly.
Ideally, _modules is used in favor of _components because pushing to _components on the server creates memory leaks! 96fc69b12ccf2428f65242eff045a36da8b612a9

The goal of this is to accomplish the following:

Prevent every call of withSSRContext on each request from pushing to Amplify._components, creating memory leaks.
Warn customers on the client when they register multiple versions of a category (e.g. Auth).
Simplify the Amplify.register usage to match pre-SSR implementation.
Move Amplify.register outside of module scope so that we honor sideEffects: false.

ericclemmons · 2020-09-23T22:51:30Z

Going to go with the simpler sideEffects implementation with @manueliglesias .

mordka · 2020-09-24T05:30:06Z

@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.

sammartinez · 2020-09-24T15:36:38Z

@mordka We will provide a link to the PR once we have one, we are working on it as we speak to unblock yourself.

github-actions · 2021-09-25T01:47:45Z

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 *-help channels or Discussions for those types of questions.

ericclemmons added 4 commits September 21, 2020 11:07

Revert "Register category to Amplify on import, not instantiation"

577a2c6

This reverts commit 167c787.

Only Amplify.register in the browser to prevent SSR memory leaks

9968d85

Update test to indicate Amplify.register is called per-instance

905f6ca

Fix import of browserOrNode

db83aad

ericclemmons requested review from Ashish-Nanda, elorzafe, iartemiev and manueliglesias as code owners September 21, 2020 18:42

ericclemmons added SSR Issues related to Server Side Rendering bug Something isn't working Core Related to core Amplify issues labels Sep 21, 2020

ericclemmons self-assigned this Sep 21, 2020

ericclemmons commented Sep 21, 2020

View reviewed changes

packages/pushnotification/src/PushNotification.ts Show resolved Hide resolved

ericclemmons commented Sep 21, 2020

View reviewed changes

Remove browserOrNode().isBrowser check for PushNotification

dfa2114

This only gets ran on iOS, not Web

ericclemmons requested review from Amplifiyer, amhinson and wlee221 September 21, 2020 20:01

amhinson reviewed Sep 21, 2020

View reviewed changes

packages/pushnotification/__tests__/PushNotification-test.ts Outdated Show resolved Hide resolved

Update packages/pushnotification/__tests__/PushNotification-test.ts

1300192

Co-authored-by: Alex Hinson <[email protected]>

wlee221 approved these changes Sep 21, 2020

View reviewed changes

Fix references to JS.browserOrNode so mocks work

5f2e531

ericclemmons force-pushed the 6761-fix-ssr-amplify-register branch from ed5cf13 to 5f2e531 Compare September 22, 2020 00:42

ericclemmons added 2 commits September 22, 2020 10:14

Amplify.register is called in every environment but node

79b57ee

Ensure browserOrNode returns isBrowser: true & isNode: false

68e1875

ericclemmons closed this Sep 23, 2020

manueliglesias mentioned this pull request Sep 24, 2020

fix: Add files with Amplify.register to sideEffects array #6867

Merged

ericclemmons mentioned this pull request Aug 17, 2021

I18n setup aws-amplify/amplify-ui#189

Merged

github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2021

	amplify.register(new m.constructor());
	}
	});

	// Associate new module instances with this amplify
	modules.forEach(m => {
	amplify.register(new m.constructor());

		if (JS.isEmpty(config)) return this._config;
		if (isEmpty(config)) return this._config;

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

Uh oh!

Conversation

ericclemmons commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ericclemmons Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Sep 21, 2020

Uh oh!

lgtm-com bot commented Sep 21, 2020

Uh oh!

amhinson Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

ericclemmons Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ericclemmons commented Sep 21, 2020

Uh oh!

lgtm-com bot commented Sep 21, 2020

Uh oh!

Amplifiyer commented Sep 21, 2020

Uh oh!

ericclemmons commented Sep 21, 2020

Uh oh!

wlee221 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Sep 21, 2020

Uh oh!

ericclemmons commented Sep 22, 2020

Uh oh!

lgtm-com bot commented Sep 22, 2020

Uh oh!

ericclemmons commented Sep 22, 2020

Uh oh!

ericclemmons commented Sep 22, 2020

Uh oh!

ericclemmons commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Simple steps for fixing Angular amplify bundling behavior:

Proposed steps for "reverting" Amplify.register behavior:

The goal of this is to accomplish the following:

Uh oh!

ericclemmons commented Sep 23, 2020

Uh oh!

mordka commented Sep 24, 2020

Uh oh!

sammartinez commented Sep 24, 2020

Uh oh!

github-actions bot commented Sep 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ericclemmons commented Sep 21, 2020 •

edited

Loading

wlee221 left a comment •

edited

Loading

ericclemmons commented Sep 23, 2020 •

edited

Loading

Proposed steps for "reverting" `Amplify.register` behavior: