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 SW note to firebase-auth #113

Merged
merged 2 commits into from
Oct 10, 2016
Merged

Conversation

robdodson
Copy link
Contributor

No description provided.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@robdodson
Copy link
Contributor Author

@@ -45,6 +45,12 @@

The `user` property will automatically be populated if an active session is
available, so handling the resolved promise of sign-in methods is optional.

It's important to note that if you're using a Service Worker you should let
urls that contain `/__auth/` go through to the network, rather than have the
Copy link
Contributor

Choose a reason for hiding this comment

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

URLs that start with /__/ should be the actual pattern, as the entire __ namespace is reserved for Firebase.

Copy link
Contributor

Choose a reason for hiding this comment

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

It may also be worth noting that this is a particular problem when your Service Worker and your site are both served from a Firebase Hosting domain. If you are self-deploying your app to some non-Firebase domain, this shouldn't be a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh interesting I didn't know that last point @cdata. I'll update the PR

@robdodson
Copy link
Contributor Author

k updated the language

@cdata is the issue the firebase domain or firebase hosting itself? Like what if I setup a custom domain for firebase hosting, would this no longer be an issue?

@cdata
Copy link
Contributor

cdata commented Sep 9, 2016

@rob I think if you have a custom domain, it's also fine. The issue with hosting is that the OAuth pop-up is loading from the same origin as the one your Service Worker is scoped to. But, it's possible that if you use a custom domain, the OAuth popup will still use the Firebase domain (which means the Service Worker won't see that network request). I haven't personally used a custom domain with Firebase Hosting, so I can't say for sure. @mbleigh might know more.

@mbleigh
Copy link
Contributor

mbleigh commented Sep 9, 2016

Custom domain will also have this issue.

@mbleigh mbleigh merged commit 07c6354 into FirebaseExtended:master Oct 10, 2016
@aicos
Copy link

aicos commented Mar 25, 2017

Good that a text has been added to firebase-auth.html, but I personally don't think its enough. I didn't see it and lost a ton of time on the issue, and think it will be the same for others too.

Maybe I'm alone on this, but I'm thinking it maybe would be good to add something into some log message somewhere ("firebase deploy" or "polymer build") that makes it more obvious so that people can resolve it faster. Or maybe add something into the timeout error message that appears in the browser console.

@robdodson
Copy link
Contributor Author

yeah it should at least be in the readme for the element as well

@robdodson
Copy link
Contributor Author

@mbleigh is the readme generated from the comments?

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.

5 participants