Skip to content

Conversation

@mmermerkaya
Copy link
Contributor

@mmermerkaya mmermerkaya commented Apr 18, 2018

Checks browser support before instantiating messaging service controllers and fixes some tests.

I think it's better to let the developer know that Messaging will not work as soon as possible, instead of instantiating a controller without an issue and then throwing an error as soon as they try to use it.

This also prevents issues such as #658 by making sure we don't call any unsupported methods by accident.

This can break users who were not checking browser support before calling firebase.messaging(), so it should go to the v5 branch.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@mmermerkaya mmermerkaya changed the base branch from master to v5/master April 24, 2018 12:24
@mmermerkaya
Copy link
Contributor Author

mmermerkaya commented Apr 24, 2018

I changed the base to v5/master, but that got @googlebot a bit confused. All commits in this PR are mine.

Copy link

@gauntface gauntface left a comment

Choose a reason for hiding this comment

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

I'm still against this PR without an API review for the permission change.

@mmermerkaya
Copy link
Contributor Author

Permission change?

Sure, I can send it to API review.

mmermerkaya pushed a commit that referenced this pull request Apr 24, 2018
mmermerkaya added a commit that referenced this pull request Apr 24, 2018
@mmermerkaya mmermerkaya force-pushed the mmermerkaya-break-early branch from 502ff89 to b0aac54 Compare May 1, 2018 11:48
@googlebot
Copy link

CLAs look good, thanks!

@jshcrowthe jshcrowthe changed the base branch from v5/master to master May 1, 2018 18:38
@mmermerkaya mmermerkaya force-pushed the mmermerkaya-break-early branch from b0aac54 to d4d4254 Compare May 2, 2018 12:42
@mmermerkaya mmermerkaya force-pushed the mmermerkaya-break-early branch from d4d4254 to e1b2d2b Compare May 2, 2018 19:25
@mmermerkaya
Copy link
Contributor Author

Went through API review, implemented requested changes.

@mmermerkaya mmermerkaya merged commit 7764746 into master May 3, 2018
@mmermerkaya mmermerkaya deleted the mmermerkaya-break-early branch May 3, 2018 09:11
@firebase firebase locked and limited conversation to collaborators Oct 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants