Skip to content
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

Add hook for web view closing #754

Merged

Conversation

xavierLowmiller
Copy link
Contributor

@xavierLowmiller xavierLowmiller commented Jan 27, 2023

  • All new/changed/fixed functionality is covered by tests (or N/A)
  • I have added documentation for all new/changed functionality (or N/A)

(Draft PR to discuss the general approach. If you're in favor of this change, I can add tests.)

📋 Changes

We're looking for a possibility to hook into the provider life cycle, in our case to show a loading animation after the web view is dismissed:

graph
  a[ASWebAuthenticationSession runs] --> b[AuthTransaction.resume runs];
Loading

This PR adds an optional closure to be executed when the web view finishes:

graph
  a[ASWebAuthenticationSession runs] --> b[onWebViewClose is called];
  b --> c[AuthTransaction.resume runs]
Loading

🎯 Testing

  1. Create a web auth session using Auth0.webAuth(...)
  2. Chain onWebViewClose on it
  3. Start the session

The onWebViewClose is called when the provider finishes its work but before the auth transaction.

@Widcket
Copy link
Contributor

Widcket commented Feb 11, 2023

Hi @xavierLowmiller, thanks for raising this and apologies for the delay.

I'll need to take a look at this in more detail, I expect to get back to this next week as I'm a bit busy at the moment. Thanks for your patience.

@Widcket Widcket added the needs investigation An issue that has more questions to answer or otherwise needs work to fully understand the issue label Feb 11, 2023
@xavierLowmiller xavierLowmiller changed the title Add hook for web view closure Add hook for web view closing Feb 13, 2023
@xavierLowmiller
Copy link
Contributor Author

xavierLowmiller commented Feb 13, 2023

Thank you!
Please take your time, don't want to make you even busier ✌️

///
/// - Parameter onWebViewClose: A closure to be executed
/// - Returns: The same `WebAuth` instance to allow method chaining.
func onWebViewClose(_ onWebViewClose: (() -> Void)?) -> Self
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call it onClose, and the parameter callback.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in ce72102 👍

Auth0/WebAuth.swift Outdated Show resolved Hide resolved
Auth0/WebAuth.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@Widcket Widcket left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @xavierLowmiller, thanks for your patience and contribution.

I see the use case, and agree this would be a good addition. I've left a couple of comments about the naming. Please don't forget to add the respective unit tests.

@Widcket Widcket added waiting for customer This issue is waiting for a response from the issue or PR author and removed needs investigation An issue that has more questions to answer or otherwise needs work to fully understand the issue labels Feb 27, 2023
@xavierLowmiller xavierLowmiller force-pushed the feature/hook-in-to-web-session-live-cycle branch from 02c61ed to c22a360 Compare February 28, 2023 14:02
xavierLowmiller and others added 2 commits February 28, 2023 15:08
Co-authored-by: Rita Zerrizuela <zeta@widcket.com>
@xavierLowmiller xavierLowmiller force-pushed the feature/hook-in-to-web-session-live-cycle branch from c22a360 to a5596d8 Compare February 28, 2023 15:44
@xavierLowmiller
Copy link
Contributor Author

Hey @Widcket!

Thank you for the review, and sorry it took so long to address your feedback.

Please don't forget to add the respective unit tests.

Please check if my additions in a5596d8 are sufficient.

@xavierLowmiller xavierLowmiller marked this pull request as ready for review February 28, 2023 15:47
@xavierLowmiller xavierLowmiller requested a review from a team as a code owner February 28, 2023 15:47
Auth0/WebAuth.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@Widcket Widcket left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @xavierLowmiller!

@Widcket Widcket removed the waiting for customer This issue is waiting for a response from the issue or PR author label Feb 28, 2023
@Widcket Widcket merged commit 749be0c into auth0:master Feb 28, 2023
@Widcket Widcket mentioned this pull request Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants