Requesting review for SwiftUI Passwordless demo app #1015

ncooke3 · 2020-07-17T22:19:56Z

Not sure where this project will live yet (will talk with dev rel) but for the sake of review, I've created a swiftui directory in this repo for this review (and to potentially house future SwiftUI projects?).

Please Review:
• PasswordlessSwiftUIApp.swift
• ContentView.swift

TODO:

README

ncooke3

Leaving a few comments.

ncooke3 · 2020-07-17T22:24:26Z

swiftui/PasswordlessSwiftUI/PasswordlessSwiftUI/ContentView.swift

+    .onOpenURL { (url) in
+      let link = url.absoluteString
+      if Auth.auth().isSignIn(withEmailLink: link) {
+        passwordlessSignIn(email: email, link: link) { (success) in
+          self.presentSheet = success
+        }
+      }
+    }


I did not call any Dynamic Links API's. Interestingly the above code works well regardless. If we use the dynamic link APIs to verify the incoming link is indeed a dynamic link, the approach below works:

// The above approach does not use the dynamic link APIs. .onOpenURL { (url) in DynamicLinks.dynamicLinks().handleUniversalLink(url) { (dynamicLink, error) in guard error == nil, let link = dynamicLink?.url?.absoluteString else { print(error!) return } if Auth.auth().isSignIn(withEmailLink: link) { passwordlessSignIn(email: email, link: link) { (success) in self.presentSheet = success } } } }

After chatting with DL team, it seems like calling handleUniversalLink() does handles reopening attribution and some analytics for the link. I'm unsure whether it is still necessary here though (because it works well without it so I'm not sure about issues arising when it is left out). Thoughts?

I'm also unsure on this bit, but we should chat about it further to understand the implications. Also to better understand: could we split those two calls up? Not saying it's the right idea, just curious.

.openURL { url in // Handle universal link DynamicLinks.dynamicLinks().... } .openURL { url in guard Auth.auth().isSignin(withEmailLink: url) else { return } // Handle auth sign in }

ncooke3 · 2020-07-17T22:26:22Z

swiftui/PasswordlessSwiftUI/PasswordlessSwiftUI/ContentView.swift

+// MARK: Example Auth View Modifier
+
+extension View {
+  func handlePasswordlessLogin(forEmail email: String,
+                               completion: @escaping (Bool)->()) -> some View {
+      self.onOpenURL { (url) in
+        let link = url.absoluteString
+        if Auth.auth().isSignIn(withEmailLink: link) {
+          passwordlessSignIn(email: email, link: link) { (success) in
+            completion(success)
+          }
+        } else {
+          completion(false)
+        }
+      }
+    }
+}


Perhaps Firebase could start including view modifiers like this one, for example.

/// It's also possible to refactor the code on line 41 by using a view modifier like: .handlePasswordlessLogin(forEmail: email) { (success) in self.presentSheet = success }

I'm definitely intrigued by this! I think it would make sense to do since it provides a lot more context than openURL and the (arguably) boiler plate code to handle the sign in.

paulb777

Generally LGTM

Leaving approval to @ryanwilson and directory structure question to @morganchen12

paulb777 · 2020-07-21T01:22:13Z

swiftui/PasswordlessSwiftUI/Podfile

@@ -0,0 +1,21 @@
+# Uncomment the next line to define a global platform for your project
+platform :ios, '14.0'


Is 13.0 possible?

14 is required for the new @main and entire-SwiftUI lifecycle (instead of AppDelegate)

ryanwilson

Looking pretty good so far! Added some comments, also it would be helpful to have screenshots uploaded as well for each revision (that changes the UI at all) to better match between the two.

ryanwilson · 2020-07-20T23:24:05Z

swiftui/PasswordlessSwiftUI/PasswordlessSwiftUI/ContentView.swift

@@ -0,0 +1,181 @@
+//


Nit: can you replace this and the other headers with the full copyright?

ryanwilson · 2020-07-20T23:24:40Z

swiftui/PasswordlessSwiftUI/PasswordlessSwiftUI/ContentView.swift

+        Spacer()
+          .frame(height: 0.0)
+


What's the point here in having a spacer with a specified 0 height in a VStack?

ryanwilson · 2020-07-20T23:27:15Z

swiftui/PasswordlessSwiftUI/PasswordlessSwiftUI/ContentView.swift

+    NavigationView {
+      VStack(spacing: 50.0) {
+        Text("Authenticate users with only their email, no password required!")
+          .frame(maxWidth: .infinity, alignment: .leading)


Is this necessary? Or what are you trying to accomplish here? If it was truncating the width, you can likely get away with setting the maximum number of lines to 0.

ryanwilson · 2020-07-20T23:27:35Z

swiftui/PasswordlessSwiftUI/PasswordlessSwiftUI/ContentView.swift

+/// Main view where user can login in using Email Link Authentication.
+struct ContentView: View {
+  @State private var email: String = ""
+  @State private var presentSheet = false


Since this is stateful, having it isPresentingSheet probably makes more sense.

ryanwilson · 2020-07-20T23:28:24Z

swiftui/PasswordlessSwiftUI/PasswordlessSwiftUI/ContentView.swift

+      }
+      .padding()
+      .navigationBarTitle("Passwordless Login")
+


Nit: empty space.

ryanwilson · 2020-07-21T01:25:44Z

swiftui/PasswordlessSwiftUI/PasswordlessSwiftUI/ContentView.swift

+    .onOpenURL { (url) in
+      let link = url.absoluteString
+      if Auth.auth().isSignIn(withEmailLink: link) {
+        passwordlessSignIn(email: email, link: link) { (success) in
+          self.presentSheet = success
+        }
+      }
+    }


I'm also unsure on this bit, but we should chat about it further to understand the implications. Also to better understand: could we split those two calls up? Not saying it's the right idea, just curious.

.openURL { url in // Handle universal link DynamicLinks.dynamicLinks().... } .openURL { url in guard Auth.auth().isSignin(withEmailLink: url) else { return } // Handle auth sign in }

ryanwilson · 2020-07-21T01:26:08Z

swiftui/PasswordlessSwiftUI/PasswordlessSwiftUI/ContentView.swift

+
+  }
+


Nit: two extra blank lines that can be removed.

ryanwilson · 2020-07-21T01:27:01Z

swiftui/PasswordlessSwiftUI/PasswordlessSwiftUI/ContentView.swift

+struct CustomStyledTextField: View {
+  @Binding var text: String
+  let placeholder: String
+  let symbol: String


Nit: symbolName, to make sure someone doesn't try to pass in unicode characters as the symbols :P

ryanwilson · 2020-07-21T01:40:24Z

swiftui/PasswordlessSwiftUI/PasswordlessSwiftUI/PasswordlessSwiftUIApp.swift

+@main
+struct PasswordlessSwiftUIApp: App {
+
+  init() {


Nit: Can you add a comment here saying this is the new place to add FirebaseApp.configure() since there's no more appDelegate?

For Firebase features that make use of AppDelegate swizzling, developers will probably need to use @UIApplicationDelegateAdaptor injection. Hence, I'd remove "recommended" from the comment.

ryanwilson · 2020-07-21T01:45:00Z

swiftui/PasswordlessSwiftUI/PasswordlessSwiftUI/ContentView.swift

+// MARK: Example Auth View Modifier
+
+extension View {
+  func handlePasswordlessLogin(forEmail email: String,
+                               completion: @escaping (Bool)->()) -> some View {
+      self.onOpenURL { (url) in
+        let link = url.absoluteString
+        if Auth.auth().isSignIn(withEmailLink: link) {
+          passwordlessSignIn(email: email, link: link) { (success) in
+            completion(success)
+          }
+        } else {
+          completion(false)
+        }
+      }
+    }
+}


I'm definitely intrigued by this! I think it would make sense to do since it provides a lot more context than openURL and the (arguably) boiler plate code to handle the sign in.

morganchen12 · 2020-07-21T16:07:54Z

Directory structure LGTM. Once we have full coverage of SwiftUI samples we can move the SwiftUI samples to root and put the old samples in a subdirectory for better discoverability.

ryanwilson · 2020-07-22T13:55:23Z

swiftui/PasswordlessSwiftUI/PasswordlessSwiftUI/ContentView.swift

        CustomStyledButton(title: "Send Sign In Link") {
-          sendSignInLink(to: email)
+          sendSignInLink()
        }


With no email now, we should be able to just use this function as an argument:

CustomStylesButton(title: "Send Sign In Link", action: sendSigninLink)

ryanwilson · 2020-07-22T14:02:25Z

swiftui/PasswordlessSwiftUI/PasswordlessSwiftUI/ContentView.swift

+        passwordlessSignIn(email: email, link: link) { user, error in
+          isPresentingSheet = user?.isEmailVerified ?? false
        }
      }
    }
-    .sheet(isPresented: $presentSheet) {
+    .sheet(isPresented: $isPresentingSheet) {
      SuccessView(email: $email)
    }
+  }


Just thought of a potential race condition here: if someone sends a sign in link, changes the email in the text field, then goes to their email and logs in, we'll show that they're logged in as the content of the text field, not the email address they actually used.

Can you think of some ways to get around this without changing the UI?

Update. When this code is triggered on a URL open, the email passed into passwordlessSignIn will be the current email typed in the textfield. passwordlessSignIn really just wraps Auth's "sign in with email and link method", which matches up the link and email and will return an error if the email and email the link was sent to don't match.

So I dont think this race condition will happen because if a user enters email A and sends the link. Then changes the textfield to email B and then goes to open the link sent to email A. They will not be signed in when returning to the app because the link corresponding with email A no longer corresponds to the binded email property, which is now email B.

One could always keep the latest email the link was sent to when a user taps the "send sign in link" button. But then what if they send the link to 5 emails and only open the link on the 1,2,3, or 4th? So I think this might be ok to leave as it with the idea that whatever email is in the textfield should represent the email the users wants to login in with.

ryanwilson · 2020-07-22T14:04:57Z

swiftui/PasswordlessSwiftUI/PasswordlessSwiftUI/ContentView.swift

  }

+  private func passwordlessSignIn(email: String, link: String,
+                                  completion: @escaping (_ user: User?, _ error: Error?) -> ()) {


We should take advantage of the Result type here instead of providing two optionals in the callback.

ryanwilson · 2020-07-22T14:06:17Z

swiftui/PasswordlessSwiftUI/PasswordlessSwiftUI/ContentView.swift

+      Spacer()
+      Text(title)
+        .padding()
+        .accentColor(.white)
+      Spacer()


Does this work as is? It's unclear how these are assembled - is this an HStack, VStack, ZStack? I'm assuming it's just a Group but it's not clear.

ryanwilson · 2020-07-24T14:24:31Z

swiftui/PasswordlessSwiftUI/PasswordlessSwiftUI/ContentView.swift

+        completion(.failure(error))
+      } else {
+        print("✔ Authentication was successful.")
+        completion(.success(Auth.auth().currentUser))


This could be result?.user instead of getting the singleton again.

This doesn't need changing right now, just wanted to point out this is an example where our API could be improved - right now you're forced to return an optional User object (or you could force unwrap it) when there:should be only 2 use cases: successfully signed in, or an error.

If we wanted to handle this case fully, we could check if result is also nil and wrap our own custom error, returning it in the completion handler. Don't think it's fully necessary but thought it was a good learning opportunity.

yes- i understand!

ryanwilson · 2020-07-24T14:25:09Z

swiftui/PasswordlessSwiftUI/PasswordlessSwiftUI/ContentView.swift

+      HStack {
+        Spacer()
+        Text(title)
+          .padding()
+          .accentColor(.white)
+        Spacer()
+      }


Nit: can you add a comment about why you're wrapping the text with two spacers in an H stack?

ryanwilson · 2020-07-24T14:26:03Z

swiftui/PasswordlessSwiftUI/PasswordlessSwiftUI/ContentView.swift

-    }
+  static var previews: some View {
+    ContentView()
+  }
 }

 // MARK: Example Auth View Modifier

 extension View {
  func handlePasswordlessLogin(forEmail email: String,


I would say if we're going to use this, let's keep it, otherwise let's delete it.

I'm good if you want to use it - you may need to change the String argument to Binding<String> though (I think...)

ryanwilson

LGTM - thanks for working through the comments! Not sure what we want to do yet with the FDL changes but I'd be fine landing this then adding it after if it's necessary.

peterfriese · 2020-07-27T10:17:42Z

swiftui/PasswordlessSwiftUI/PasswordlessSwiftUI/PasswordlessSwiftUIApp.swift

+@main
+struct PasswordlessSwiftUIApp: App {
+
+  /// With the removal of `AppDelegate`, it is recommended to configure Firbase here.


Typo: Firbase -> Firebase

peterfriese · 2020-07-27T10:21:06Z

swiftui/PasswordlessSwiftUI/PasswordlessSwiftUI/ContentView.swift

+
+  private func sendSignInLink() {
+    let actionCodeSettings = ActionCodeSettings()
+    actionCodeSettings.url = URL(string: "https://passwordlessswiftui.page.link/demo_login")


I wonder if it might be a good idea to extract project-dependent constants and move them to a configuration struct that developers can then update with their project-specific values.

As far as project constants, I can think of the above URL and some of the button titles, textfield placeholders, etc. Are these items I should move to a configuration struct?

I was thinking mostly of the URL, which would also benefit from being decomposed into the prefix and short link. Button titles etc. should be dealt with proper i18n tools (which might be beyond the scope of this sample, but I'll leave it to @ryanwilson to comment on how stringent we are with i18n in our code samples).

Yep I think we can leave the i18n out for this - breaking the project constant out to a separate file SGTM, no other variables are needed there.

ncooke3 · 2020-07-28T21:01:22Z

@ryanwilson Following up on our offline discussion yesterday about error handling... What are your thoughts on this latest commit? I experimented with a few things but thought this may be a nice way to display errors using Alert?

ryanwilson · 2020-07-29T13:42:08Z

@ryanwilson Following up on our offline discussion yesterday about error handling... What are your thoughts on this latest commit? I experimented with a few things but thought this may be a nice way to display errors using Alert?

I like it - thanks for doing so!

ryanwilson

Just a few small formatting nits, ~~can you change this from a draft request so the other GitHub checks run?~~ I converted it and forgot it's only Travis running.

ryanwilson · 2020-07-31T13:22:28Z

swiftui/PasswordlessSwiftUI/README.md

+new lifeycle [APIs](https://developer.apple.com/documentation/swiftui/app) 
+that enabled developers to build apps entirely with 
+[SwiftUI](https://developer.apple.com/xcode/swiftui/). This project uses these 
+latest iOS14 APIs in conjunction with Firebase to provide a clear 


Nit: space between iOS and 14

ryanwilson · 2020-07-31T13:22:40Z

swiftui/PasswordlessSwiftUI/README.md

@@ -0,0 +1,42 @@
+# Using Firebase Auth's Email Link Login with SwiftUI


Nit: newline after the #

ryanwilson · 2020-07-31T13:22:52Z

swiftui/PasswordlessSwiftUI/README.md

+latest iOS14 APIs in conjunction with Firebase to provide a clear 
+example of how to build modern apps with Firebase and SwiftUI.
+
+# Getting Started


Nit: newline after the #

ryanwilson · 2020-07-31T13:23:00Z

swiftui/PasswordlessSwiftUI/README.md

+Getting this project up and running is similar to the other quickstarts 
+found in this repo. The following steps will clone the project, 
+install its dependencies, and open the `.xcworkspace` project in Xcode.
+```bash


Nit: newline before this.

ryanwilson · 2020-07-31T13:23:34Z

swiftui/PasswordlessSwiftUI/README.md

+$ open PasswordlessSwiftUI.xcworkspace
+```
+
+# Finished Demo


Nit: newline after the #

Requesting review for SwiftUI Passwordless demo app

b5aec4f

ncooke3 requested review from paulb777 and ryanwilson July 17, 2020 22:19

ncooke3 commented Jul 17, 2020

View reviewed changes

paulb777 reviewed Jul 21, 2020

View reviewed changes

ryanwilson reviewed Jul 21, 2020

View reviewed changes

ncooke3 added 2 commits July 21, 2020 19:07

Resolved feedback

90c701b

Adding media folder for UI pics

43e8f97

ryanwilson reviewed Jul 22, 2020

View reviewed changes

Resolved feedback 2.0

235e68d

ryanwilson reviewed Jul 24, 2020

View reviewed changes

Resolved feedback 3.0

b973806

ryanwilson approved these changes Jul 24, 2020

View reviewed changes

peterfriese reviewed Jul 27, 2020

View reviewed changes

ncooke3 and others added 2 commits July 27, 2020 08:58

Update comment in PasswordlessSwiftUI.app

91a5243

Improved error handling

e21541e

ncooke3 requested a review from ryanwilson July 28, 2020 20:59

ncooke3 and others added 6 commits July 31, 2020 00:58

Readme and gif

e0cd35a

Readme gif format

41206d4

Gif linking

a4af2d2

Gif links again

fcd2ae5

finalize gif relative link

8c13231

Fix license spacing

f5d9161

ryanwilson reviewed Jul 31, 2020

View reviewed changes

ryanwilson marked this pull request as ready for review July 31, 2020 14:09

Update README.md

d521258

ryanwilson approved these changes Jul 31, 2020

View reviewed changes

ncooke3 merged commit 78b3962 into master Jul 31, 2020

ncooke3 deleted the nc-passwordless-swiftui branch December 18, 2023 17:38

		@@ -0,0 +1,21 @@
		# Uncomment the next line to define a global platform for your project
		platform :ios, '14.0'

		@@ -0,0 +1,42 @@
		# Using Firebase Auth's Email Link Login with SwiftUI

Requesting review for SwiftUI Passwordless demo app #1015

Requesting review for SwiftUI Passwordless demo app #1015

Conversation

ncooke3 commented Jul 17, 2020 • edited Loading

ncooke3 left a comment

Choose a reason for hiding this comment

ncooke3 Jul 17, 2020 • edited Loading

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

paulb777 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanwilson 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morganchen12 commented Jul 21, 2020

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

Choose a reason for hiding this comment

ryanwilson 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

ncooke3 commented Jul 28, 2020 • edited Loading

ryanwilson commented Jul 29, 2020

ryanwilson left a comment • edited Loading

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

ncooke3 commented Jul 17, 2020 •

edited

Loading

ncooke3 Jul 17, 2020 •

edited

Loading

ncooke3 commented Jul 28, 2020 •

edited

Loading

ryanwilson left a comment •

edited

Loading