Skip to content

Conversation

@dakeshi
Copy link
Contributor

@dakeshi dakeshi commented Aug 4, 2016

optional sourceApplication has to use with the optional binding to prevent the optional syntax error.

With the optional binding, sourceApplication which is the handleOpenURL method argument is always has a value.

Is there another solution?

@morganchen12
Copy link
Contributor

This code always returns false if there was no source application even if the app can handle the available URL, which is not desired behavior. The ugly workaround with sourceApplication ?? "" tries to resolve this by hoping every URL handler treats empty or unrecognized source applications as they treat nil, which is closer to but not necessarily the desired behavior. Ideally all of the URL handlers should know how to deal with nil.

We'll be able to eventually remove this since #100 was merged, but for now I'd prefer the ?? "" workaround.

@dakeshi
Copy link
Contributor Author

dakeshi commented Aug 8, 2016

Thank you for your feedback @morganchen12.

This code always returns false if there was no source application even if the app can handle the available URL, which is not desired behavior.

It sounds like that handleOpenURL method of the authUI works well regardless sourceApplication argument value. If so, I think that handleOpenURL method api need to be updated to have the only one argument: the type of NSURL.

And then we'll have more simpler syntax:

    if FIRAuthUI.authUI()?.handleOpenURL(url) {
      return true
    }

    return false
...

Is it important to get sourceApplication argument in handleOpenURL method?

but for now I'd prefer the ?? "" workaround.

Yes, it would be nicer solution temporally.

@morganchen12
Copy link
Contributor

handleOpenURL calls through to a protocol method on each of the login provider objects that the FIRAuthUI instance knows about, so having a sourceApplication could be useful to them (i.e. Facebook auth might expect that its callback came from the Facebook app or Safari and fail otherwise). But the optionality is definitely incorrect currently, and will be fixed in the next release.

use nil coalescing operator for sourceApplication argument.
@dakeshi
Copy link
Contributor Author

dakeshi commented Aug 10, 2016

Fixed it : a5197f6

Remove the optional binding and use sourceApplicaiton ?? ""

@morganchen12
Copy link
Contributor

Thanks!

@morganchen12 morganchen12 merged commit fe24c52 into firebase:master Aug 10, 2016
@dakeshi dakeshi deleted the fix_optional_error branch August 16, 2016 23:59
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.

2 participants