-
Notifications
You must be signed in to change notification settings - Fork 44
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
implement app state getter for web #509
base: main
Are you sure you want to change the base?
implement app state getter for web #509
Conversation
@Widcket Apologies, I had an issue during the interactive rebase when trying to fix the commit signing, so I had to start over. I also applied your comments from the previous PR into this one. |
/// Get the app state that was provided during a previous call | ||
/// to [loginWithRedirect]. | ||
/// | ||
/// This method should be called after calling [onLoad]. |
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.
The note about second access returning null is now removed.
/// Thus clearing this object is not needed, | ||
/// as the actual state is managed across reloads, | ||
/// using the transaction manager. | ||
// TODO: move the `appState` to the result of `onLoad/initialize` |
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.
Please let me know if I need to update this new TODO
Object? _appState; | ||
|
||
@override | ||
Future<Object?> get appState => Future<Object?>.value(_appState); |
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.
Per your request, the getter now returns the value as-is
@@ -21,6 +21,7 @@ dependency_overrides: | |||
|
|||
dev_dependencies: | |||
build_runner: ^2.1.8 | |||
collection: ^1.18.0 |
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 for the MapEquality
in tests.
@@ -15,6 +15,10 @@ abstract class Auth0FlutterWebPlatform extends PlatformInterface { | |||
_instance = instance; | |||
} | |||
|
|||
Future<Object?> get appState { |
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.
Previously there was also an @override
that duplicated this implementation. I removed that one.
Maybe that is why the code coverage report was reporting the uncovered lines?
|
||
when(mockClientProxy.isAuthenticated()) | ||
.thenAnswer((final _) => Future.value(false)); | ||
when(mockClientProxy.handleRedirectCallback()) |
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 test was updated to verify the redirect callback as well. (since it now returns a value)
); | ||
}); | ||
|
||
test('appState getter returns value when accessed more than once', () async { |
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 test was added to prevent regressing the behavior of the new getter. (it should always return the same value)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #509 +/- ##
=============================================
- Coverage 96.08% 83.97% -12.12%
=============================================
Files 97 108 +11
Lines 1611 1884 +273
Branches 331 424 +93
=============================================
+ Hits 1548 1582 +34
- Misses 49 289 +240
+ Partials 14 13 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I don't understand why the Sentry report might still be reporting the getter as uncovered? |
It seems that the upstream iOS & MacOS tests are broken currently? This prevents new test reports from being made. |
@@ -32,7 +32,6 @@ linter: | |||
test_types_in_equals: true | |||
throw_in_finally: true | |||
unnecessary_statements: true | |||
unsafe_html: true |
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.
unsafe_html
is removed in Dart 3.7. The project had no ignores, so this can be removed.
@@ -91,4 +90,4 @@ linter: | |||
sort_pub_dependencies: true | |||
|
|||
# Specific rules | |||
package_api_docs: true | |||
package_api_docs: true # TODO: replace with public_member_api_docs |
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 lint is removed in Dart 3.7, I added a TODO for the most likely replacement.
Enabling that lint is more work, though.
external int get arrayLength; | ||
|
||
@JS('at') | ||
external JSFunction get elementAt; |
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.
Overriding operator[]
didn't help, because then I still get a warning.
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.
Can this be moved to the js_interop file ?
@@ -27,4 +30,22 @@ void main() { | |||
expect((output as dynamic).custom_param, null); | |||
}); | |||
}); | |||
|
|||
group('stripNulls', () { | |||
test('removes null values from an object', () { |
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.
Regression test for the change in stripNulls.
/// | ||
/// See https://api.dart.dev/dart-js_interop/NullableObjectUtilExtension/jsify.html | ||
Future<void> loginWithRedirect({ | ||
final Object? appState, |
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.
I think it will be easier if appState is passed as the last option with a default value so it doesn't break for any existing users
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.
These are named parameters, so the position of the parameter does not matter (you need to specify it as loginWithRedirect(appState: myStateObject)
)
Since this is already nullable, this does not need a default value (the default is null)
}) => | ||
Auth0FlutterWebPlatform.instance.loginWithRedirect( | ||
LoginOptions( | ||
appState: appState, |
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.
Same as above . Can we move this as an optional parameter with a default value
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.
LoginOptions.appState is already a named nullable argument, so the position is irrelevant and it has a default (null).
|
||
_appState = JsInteropUtils.dartifyObject(result.appState); | ||
|
||
return; |
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.
Shouldn't the result be returned here and the return type be Future ?
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.
That would be a breaking change for the Auth0Web
interface. See also the TODO I added on line 36 in this file.
For now users can keep calling initialize(), while retrieving the value using the getter.
external factory RedirectLoginOptions( | ||
{final AuthorizationParams authorizationParams, final String fragment}); | ||
external factory RedirectLoginOptions({ | ||
final JSAny? appState, |
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 optional right ? Would it be easier if we have a secondary constructor so no existing implementation breaks and users can easily migrate ?
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.
The RedirectLoginOptions
is a JS interop type and is an implementation detail that is not directly used by users.
final key = objKeys[i]; | ||
for (var i = 0; i < objKeys.arrayLength; i++) { | ||
// TODO: replace w/ `final key = objKeys[i];` when updating to Dart 3.6.0 | ||
final key = objKeys.elementAt.callAsFunction(objKeys, i.toJS)!; |
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.
why do we need this change ?
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.
The []
operator and the length
getter on JSArray are only available from Dart 3.6.0 onwards.
However, requiring to go from Dart 3.5.0 to Dart 3.6.0 would imply a switch from Flutter 3.24 to 3.27.
Therefor I opted to use a backwards compatible fix for now.
/// that do not fit into a static interop definition. | ||
/// | ||
/// See https://api.dart.dev/dart-js_interop/NullableObjectUtilExtension/jsify.html | ||
static JSAny? jsifyObject(final Object? obj) => obj.jsify(); |
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.
@navaronbracke Do we need these helper methods ? Wouldn't be much easier if we call .jsify() and .dartify() directly on the corresponding objects
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.
Done
@pmathew92 PTAL. I updated the JS interop to call dartify/jsify directly and moved the polyfill for JSArray to the correct file. As for the For the switch to Dart 3.6.0, that is up to you, as this would mean that users that are on Flutter 3.24 (for WASM support) will need to migrate to Flutter 3.27 for this change. |
Thankyou @navaronbracke |
Any ideas why the code coverage check is failing? The indirect diff seems to be unrelated to this PR? |
Fixes #486
Replaces #485 (I had an issue with signing the commits during the interactive rebase, so I started over)
📋 Changes
This PR adds support for the
appState
parameter 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