Fix FCM service worker setup code for >= 7.0.0.

[aviaryan]

Update the FCM service worker setup code to work with >=7.0.0 Firebase JavaScript SDK. There was a major change in v7.0.0 as mentioned in the release notes.

The same has been also acknowledged in this firebase-js-sdk issue.

PS - I am not sure if the comment I have updated in this PR is used to render the official firebase.google.com docs but looking at the metadata around the comment and noticing that firebase.google.comcode block links to this commented code, I am guessing that's the case.

[jhuleatt]

Thanks @aviaryan, nice catch! Since this will affect the docs, I've set FCM experts @kroikie and @egilmorez as reviewers. Once they approve, we can merge this in.

[jhuleatt]

Another option might just be to tell developers to insert their entire Firebase config object. cc @kroikie

[jhuleatt]

Chatted with Arthur and got consensus that it is easier to tell devs to copy & paste the entire object. @aviaryan can you please change to this?

 // Initialize the Firebase app in the service worker by passing in
 // your app's Firebase config object
 // https://firebase.google.com/docs/web/setup#config-object
 firebase.initializeApp({
    apiKey: "api-key",
    authDomain: "project-id.firebaseapp.com",
    databaseURL: "https://project-id.firebaseio.com",
    projectId: "project-id",
    storageBucket: "project-id.appspot.com",
    messagingSenderId: "sender-id",
    appId: "app-id",
    measurementId: "G-measurement-id",
 });

We could also modify the code to throw an error if a proper config isn't passed in. This would have to be wrapped in snippet EXCLUDE tags so it doesn't show up in the docs, but would be nice for people running the quickstart. What do you think @aviaryan?

// Initialize the Firebase app in the service worker by passing in
// your app's Firebase config object
// https://firebase.google.com/docs/web/setup#config-object
firebase.initializeApp({
  apiKey: "api-key",
  authDomain: "project-id.firebaseapp.com",
  databaseURL: "https://project-id.firebaseio.com",
  projectId: "project-id",
  storageBucket: "project-id.appspot.com",
  messagingSenderId: "sender-id",
  appId: "app-id",
  measurementId: "G-measurement-id"
});

// [START_EXCLUDE]
if (firebase.app().options.appId === "app-id") {
  throw new Error(
    "You forgot to put a real Firebase config object in firebase-messaging-sw.js"
  );
}
// [END_EXCLUDE]

// Retrieve an instance of Firebase Messaging so that it can handle background
// messages.
const messaging = firebase.messaging();

[aviaryan]

We could also modify the code to throw an error if a proper config isn't passed in.

Good idea. I have a couple of comments on it.

1. Is there a need to do this? All previous versions of docs (I guess) didn't have apiKey, projectId and other configurations specified and it worked just fine with the users.

2. If we do this, maybe we can add a comment over that if block specifying to remove the block after the Firebase config object is placed. Something like the following. This makes it clear that the block is a temporary code and can/should be removed.

// [START_EXCLUDE]
// Remove the following block after updating the Firebase config object above

[jhuleatt]

1. It isn't necessary to include the entire config object, but the thought is "why not" - it seems easier to copy and paste the whole thing from the Firebase console (a well-documented process) than to pick out the apiKey, projectId, messagingSenderId and appId fields.
2. Sure! Sounds good to me 😀

[aviaryan]

It isn't necessary to include the entire config object, but the thought is "why not"

@jhuleatt Sorry, my #1 comment was also for the second part of your suggestion which involves checking appId for a non-default value. Since we haven't been doing this so far (providing a check for appId or firebase object), do users need this? What if it just adds extra complexity?

[jhuleatt]

Oh, I see. Yeah it isn't totally necessary, and there's a chance the code/confusion it adds might outweigh the benefit of a slightly nicer error message. It's up to you - I'm happy to merge with or without it.

[jhuleatt]

Actually, since you've got it all set with the new config object, I'm just going to merge as-is. We can always work on that check in another PR.

[aviaryan]

Thanks for merging.

... there's a chance the code/confusion it adds might outweigh the benefit of a slightly nicer error message. It's up to you - I'm happy to merge with or without it.

I think we shouldn't add it because most developers are already aware they need to put their Firebase config object (or something to authenticate their firebase app) somewhere for them to use a Firebase app. So it's unlikely they will do this mistake.

Also, the code file isn't too long for someone to miss the part that involves setting the firebase config object.

[aviaryan]

Hi, any updates on this? Can I do anything to help?

[jhuleatt]

Hey @aviaryan sorry for the delay. I added a new comment. Once you address that, we can merge :)

[aviaryan]

@jhuleatt Thanks for getting back. I have made the config object changes. I also changed double quotes to single quotes because the rest of the file was using that.

I haven't made the excluded appId checker changes yet as I am waiting for your thoughts. Posted my comments here. #422 (comment)

[jhuleatt]

Thank you @aviaryan!

[egilmorez]

LG, thanks!