-
Notifications
You must be signed in to change notification settings - Fork 43
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
[SDK-4001] Logout for web platform #223
Conversation
import 'auth0_flutter_web_platform_proxy.dart'; | ||
import 'extensions/client_options_extensions.dart'; | ||
import 'extensions/credentials_extension.dart'; | ||
import 'js_interop.dart'; | ||
import 'extensions/logout_options.extension.dart'; | ||
import 'js_interop.dart' as interop; |
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.
As we now have LogoutOptions
in both the platform interface and JS interop, I prefixed the interop types. Kind of useful to distinguish them anyway.
import 'js_interop.dart'; | ||
import 'extensions/logout_options.extension.dart'; | ||
import 'js_interop.dart' as interop; | ||
import 'js_interop_utils.dart'; |
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.
Moved stripNulls
out to a utility class so that they could also be used in extensions.
Future<void> logout(final LogoutOptions? options) { | ||
throw UnsupportedError('logout is only supported on the web platform'); | ||
} |
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 a little weird and one case where sharing the existing platform implementation might have been useful.
|
||
expect(params.federated, true); | ||
expect(params.returnTo, 'http://returnto.url'); | ||
}); |
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.
Should we add a failing test? It'll have to be changed when we add our new exception, but at least it will be in place.
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.
Also, should we test that the nulls are being stripped, like in "loginWithRedirect strips options that are null"?
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.
Should we add a failing test?
I've added one now in ab3524b that can be rewritten later 👍🏻
Also, should we test that the nulls are being stripped, like in "loginWithRedirect strips options that are null"?
Yes I agree that's a good test, and in fact started down this road as part of the login work but decided I'll revisit it later. On a dynamic
object (which is what is returned by a captured argument), there's no easy way to distinguish between something that's null
and something that's not been set. If you take this example:
final params = verify(mockClientProxy.loginWithRedirect(captureAny))
.captured
.first
.authorizationParams;
expect(params.audience, null);
params.audience
will be null
if it hasn't been set, or if it's actually been set to null
. To test stripNulls
properly we'll need to make use of Dart's reflection capabilities, the extent of which I haven't fully explored but presumably we'll be able to do some runtime dynamic trickery to figure out whether the property actually exists on params
. I didn't want to block the login feature at the time but I'll add a separate ticket for this 👍🏻
531a273
to
39f5f05
Compare
📋 Changes
This PR adds logout support for the web platform. Additional comments made below on specific code points.
📎 References
SDK-4001
in Jira.🎯 Testing
The example app was modified to support using web logout as well as on mobile.