App State Observable #168

fearnycompknowhow · 2021-06-12T00:03:30Z

Description

My changes enable direct access to the appState that is returned from the handleRedirectCallback method.
This appState is made available through a new appState$ observable on the AuthService class.

References

The need for this feature has been voiced by a few other developers: https://community.auth0.com/t/handle-appstate-and-redirect-to-dynamic-url-after-successful-login-auth0-angular/50982

Testing

You can run both the unit tests and the e2e tests the normal way

Checklist

This change adds test coverage for new/changed/fixed functionality
I have added documentation for new/changed functionality in this PR or in auth0.com/docs
All active GitHub checks for tests, formatting, and security are passing
The correct base branch is being used, if not master

frederikprijck

Thanks for the PR, Keep in mind it is encourages to open an issue first so we can discuss this before the work is done.

I do want to add that appState is already available to your application today, but I agree it's not clear on how to do so:

Start with importing AuthModule and setting skipRedirectCallback to true. This will ensure our SDK will not handle the redirect for you.

imports: [
  AuthModule.forRoot({ skipRedirectCallback: true })
]

Call handleRedirectCallback yourself where appropriate, be sure this is only on the configured /callback route:

constructor(private authService: AuthService) {
  this.authService.handleRedirectCallback()
    .pipe(map(result => result?.appState))
    .subscribe(appState => /* use appState here */);
}

As you can see, calling handleRedirectCallback yourself gives you access to the appState without introducing any new code.

That said, I agree this is not convenient and I would prefer for people to not have to need to call handleRedirectCallback themselves if all they need is access to the appState.

frederikprijck · 2021-06-14T06:58:10Z

README.md

+}
+```
+
+> This method of saving application state will save it in Session Storage which is capable of holding multiple MBs worth of data, so you should have more than enough space for your needs


This is only true by default, but it could as well be cookies if useCookiesForTransactions is used. Might also make sense to ensure it's clear that this data is only persisted temporarily and will be removed again once being redirected back to your application.

I also believe this message makes it sounds like you can just use this to track any appState you like (as we have storage for multiple MBs worth of data anyway), while I still believe your application should not solely rely on Auth0's SDK to persist your state, you could as well implement something yourself outside of our SDK which gives you more control.

I think it makes more sense to drop the last part and only mention it get stored in Session Storage by default, but can be stored into a cookie when setting useCookiesForTransactions.

Because of the fact that we can also use cookies, I would (as mentioned earlier) remove any emphasis on deeply nested objects (which is also who I would prefer to not have this end up in the readme, as there are limitations to using appState and this SDK is not a state persisting library).

frederikprijck · 2021-06-14T07:00:21Z

projects/auth0-angular/src/lib/auth.service.ts

@@ -333,7 +340,10 @@ export class AuthService implements OnDestroy {
        if (!isLoading) {
          this.refreshState$.next();
        }
-        const target = result?.appState?.target ?? '/';
+        const appState = result.appState;


Any reason why we changed result?.appState to be result.appState?

Pardon me, this was just a simple oversight on my part

frederikprijck · 2021-06-14T07:14:06Z

projects/playground/src/app/app.component.ts

@@ -36,6 +38,12 @@ export class AppComponent {
    ignoreCache: new FormControl(false),
  });

+  ngOnInit(): void {
+    this.auth.appState$.subscribe((appState) => {
+      this.appState = appState.deeply.nested.value;


I am confused with this example. The appState is the appState, and not some deeply nested value.
So I would either use this.appState = appState or this.someOtherProperty = appState.deeply.nested.value;

That said, even though it's good knowing this is possible, I would reduce the nesting here as I believe we want to avoid people using appState to track too much complexity. I would personally stick to a single property on the appState.

frederikprijck · 2021-06-14T07:15:54Z

projects/playground/src/app/app.component.ts

+              value: this.loginOptionsForm.value.appState,
+            },
+          },
+        },


Same here, it confuses me that we are using appState.deeply.nested.value = this.loginOptionsForm.value.appState, it should either be appState: appState or appState.deeply.nested.value = this.loginOptionsForm.value.someValue.

However, I said before I would prefer to reduce the nesting.

frederikprijck · 2021-06-14T07:23:51Z

projects/auth0-angular/src/lib/auth.service.ts

+        const appState = result.appState;
+        const target = appState?.target ?? '/';
+
+        this.appStateSubject$.next(appState);


Even though the comments on the appState$ mention it emits (if any), yet it emits even if appState is null or undefined.

frederikprijck · 2021-06-14T07:24:31Z

README.md

+  loginWithRedirect(): void {
+    this.auth.loginWithRedirect({
+      appState: {
+        // Specify which application state you would like to preserve here (yes, objects ARE allowed)


I would be a bit more explicit on the sample and drop the emphasis on objects here.

appState: { myValue: 'My State to Track' }

frederikprijck · 2021-06-14T07:25:46Z

README.md

+
+  ngOnInit() {
+    this.auth.appState$.subscribe((appState) => {
+      // Your value will be available here


I would show what this means instead of the comment:

this.auth.appState$.subscribe((appState) => { console.log(appState.myValue); });

frederikprijck · 2021-06-14T07:29:15Z

README.md

@@ -121,6 +121,56 @@ On your template, provide a button that will allow the user to log in to the app
 </button>
 ```

+### Preserve application state through redirects


I would move this to our FAQ as I believe this is not a main feature of our SDK. We want to avoid that people rely to heavily on our SDK for tracking application state.

fearnycompknowhow · 2021-06-14T19:25:16Z

@frederikprijck Sorry for not opening a discussion request first. I'll be sure to do that if I do any more work on this repo.

As far as the build issues go: the Cypress tests keep failing, because somewhere undefined is being typed into an input.
The only cause I can think of is that the call to Cypress.env('INTEGRATION_PASSWORD'); is returning undefined.

EMAIL and appState are the only other values that are being used for typing, but they are both defined statically, so I can't see where they would be getting set to undefined.

frederikprijck · 2021-06-14T19:47:57Z

Thanks for making the changes 🚀 🔥
I will look into the Cypress build tomorrow morning!

fearnycompknowhow · 2021-06-14T20:10:20Z

No problem. Let me know if I missed anything or if there is anything else you want changed

frederikprijck · 2021-06-15T10:12:56Z

Cypress tests run fine locally. The reason they fail here is because we do not allow running them on forks.

frederikprijck

Thanks for this PR 🔥

fearnycompknowhow · 2021-06-15T13:08:47Z

No problem. It was my pleasure

fearnycompknowhow added 2 commits June 11, 2021 18:37

Added an 'appState$' observable to the 'AuthService' class

0167c1b

Updated the README

e78a452

fearnycompknowhow requested a review from a team as a code owner June 12, 2021 00:03

fearnycompknowhow changed the title ~~App State oObservable~~ App State Observable Jun 12, 2021

Added e2e test

a57086f

fearnycompknowhow force-pushed the app-state-observable branch from 92634ae to a57086f Compare June 12, 2021 00:15

frederikprijck suggested changes Jun 14, 2021

View reviewed changes

fearnycompknowhow added 9 commits June 14, 2021 11:14

Updated notes in README

7e64bfb

Re-added optional chaining operator

ea47dbf

Removed deeply nested appState from E2E test

4e5cf86

Emit appState only when present

acff387

Changed comments to actual values in README

1699089

Moved README content to FAQ

1bd6255

Removed deeply nested value from AuthService unit test

31b2914

Fixed Playground unit test

fb294b8

Moved and renamed APP_STATE variable

d71095c

Update test name

3ab81f0

frederikprijck approved these changes Jun 15, 2021

View reviewed changes

Merge branch 'master' into app-state-observable

0602d69

frederikprijck added the CH: Added PR is adding feature or functionality label Jun 16, 2021

frederikprijck added this to the vNext milestone Jun 16, 2021

frederikprijck merged commit e9b216b into auth0:master Jun 16, 2021

crew-security mentioned this pull request May 13, 2022

[Snyk] Upgrade tslib from 2.3.1 to 2.4.0 #306

Merged

navaronbracke mentioned this pull request Nov 20, 2024

add support for app state argument auth0/auth0-flutter#485

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

App State Observable #168

App State Observable #168

fearnycompknowhow commented Jun 12, 2021

frederikprijck left a comment

frederikprijck Jun 14, 2021

frederikprijck Jun 14, 2021

fearnycompknowhow Jun 14, 2021

frederikprijck Jun 14, 2021

frederikprijck Jun 14, 2021

frederikprijck Jun 14, 2021

frederikprijck Jun 14, 2021

frederikprijck Jun 14, 2021

frederikprijck Jun 14, 2021

fearnycompknowhow commented Jun 14, 2021

frederikprijck commented Jun 14, 2021 •

edited

Loading

fearnycompknowhow commented Jun 14, 2021

frederikprijck commented Jun 15, 2021

frederikprijck left a comment

fearnycompknowhow commented Jun 15, 2021

App State Observable #168

App State Observable #168

Conversation

fearnycompknowhow commented Jun 12, 2021

Description

References

Testing

Checklist

frederikprijck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fearnycompknowhow commented Jun 14, 2021

frederikprijck commented Jun 14, 2021 • edited Loading

fearnycompknowhow commented Jun 14, 2021

frederikprijck commented Jun 15, 2021

frederikprijck left a comment

Choose a reason for hiding this comment

fearnycompknowhow commented Jun 15, 2021

frederikprijck commented Jun 14, 2021 •

edited

Loading