add support for app state argument #485
Conversation
| @@ -21,6 +21,7 @@ dependency_overrides: | |||
|
|
|||
| dev_dependencies: | |||
| build_runner: ^2.1.8 | |||
| collection: ^1.18.0 | |||
There was a problem hiding this comment.
For the MapEquality in the test, hence a dev dependency
|
It seems I still missed a step, as the app state is not passed to the redirect url when actually coming back to the app. Once that is fixed, this is ready for review. |
| @@ -20,8 +20,8 @@ class Auth0FlutterWebClientProxy { | |||
| [final GetTokenSilentlyOptions? options]) => | |||
| promiseToFuture(client.getTokenSilently(options)); | |||
|
|
|||
| Future<void> handleRedirectCallback() => | |||
| promiseToFuture(client.handleRedirectCallback()); | |||
| Future<RedirectLoginResult> handleRedirectCallback([final String? url]) => | |||
There was a problem hiding this comment.
The return value and parameters didn't match what is defined in the JS SDK.
For the fix, I need the new return value, but the url is currently unused.
Adding support for custom URL's could be done later, I guess.
| final PopupLoginOptions? options, | ||
| final PopupConfigOptions? config, | ||
| ]); | ||
| external Future<RedirectLoginResult> handleRedirectCallback([ |
There was a problem hiding this comment.
The app state is defined in the RedirectLoginResult, so we need it here too.
| /// Thus clearing this object is not needed, | ||
| /// as the actual state is managed across reloads, | ||
| /// using the transaction manager. | ||
| Object? _appState; |
There was a problem hiding this comment.
This property & getter are new. We provide these now, so that:
a) users can retrieve the app state
b) we do not need to change the API for initialize/onLoad (the app state is web only anyway? Or so I think?)
In the future, we might want to move the redirect state to the result of onLoad() / initialize() directly. That would make the API cleaner, but that does involve a breaking change, so the plugin would have to do this in version 2.0.0
| @@ -31,4 +31,22 @@ class JsInteropUtils { | |||
|
|
|||
| return obj; | |||
| } | |||
|
|
|||
| // TODO: replace with `dartify` from `dart:js_interop_unsafe` when migrating to WASM | |||
There was a problem hiding this comment.
I added these here, to make the migration to WASM easier. Then we only need to change these two, instead of all callsites
| @@ -72,13 +74,75 @@ void main() { | |||
|
|
|||
| test('handleRedirectCallback is called on load when auth params exist in URL', | |||
| () async { | |||
| final interop.RedirectLoginResult mockRedirectResult = | |||
There was a problem hiding this comment.
This test was updated. Now it also mocks handleRedirectCallback
| verifyNever(mockClientProxy.checkSession()); | ||
| }); | ||
|
|
||
| test('handleRedirectCallback captures appState that was passed', () async { |
There was a problem hiding this comment.
New test to verify behavior of the app state
| expect(eq.equals(capturedAppState, appState), isTrue); | ||
| }); | ||
|
|
||
| test('appState getter returns null when accessed more than once', () async { |
There was a problem hiding this comment.
New test to verify behavior of the app state
| @@ -160,6 +224,33 @@ void main() { | |||
| expect(params.max_age, null); | |||
| }); | |||
|
|
|||
| test('loginWithRedirect supports appState parameter', () async { | |||
There was a problem hiding this comment.
New test to verify behavior of the app state
|
Failing test was fixed, so this is ready for review! |
Fixes #486
📋 Changes
This PR adds support for the
appStateparameter tologinWithRedirect()for the web.See https://github.com/auth0/auth0-spa-js/blob/f2e566849efa398ca599daf9ebdfbbd62fcb1894/src/global.ts#L298
There is prior art in auth0/auth0-angular#168
🎯 Testing
Added several unit tests. This can be tested end-to-end by setting up a login with redirect, that uses the app state argument as parameter, and then validating that the app state is returned when the user is redirected back.
I did verify end-to-end using an internal application that uses the SDK integration.
To run the unit tests locally, use
flutter test test/web/auth0_flutter_web_test.dart --platform=chrome