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

FCM Pre Modularization #3234

Merged
merged 18 commits into from
Aug 11, 2020
Merged

FCM Pre Modularization #3234

merged 18 commits into from
Aug 11, 2020

Conversation

zwu52
Copy link
Member

@zwu52 zwu52 commented Jun 18, 2020

[ariane/4026631 IS APPROVED]

This PR implement "Milestone I" in section: API Change Plan: Deprecation, Removal, Addition. Specially, this PR:

Mark Deprecate:

  • setBackgroundHandler (instead divert developer to use onBackgroundMessage)
  • onTokenRefresh (currently no-op and we have no plan to implement in foreseeable future)
  • useVapidKey (divert developer to use getToken(options:{serviceWorkerRegistration, vapidKey})
  • useServiceWorker (divert developer to use getToken(options:{serviceWorkerRegistration, vapidKey})
  • getToken() (divert developer to use getToken(options:{serviceWorkerRegistration, vapidKey})

Add:

  • getToken(options:{serviceWorkerRegistration, vapidKey}
  • onBackgroundMessage
  • Typing for onBackgroundMessage: this would make easy for user to know what parameters are gettable.

@zwu52 zwu52 requested a review from egilmorez as a code owner June 18, 2020 16:34
@zwu52 zwu52 marked this pull request as draft June 18, 2020 16:34
@zwu52 zwu52 requested review from Feiyang1, hsubox76 and a team June 18, 2020 16:35
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jun 18, 2020

Binary Size Report

Affected SDKs

  • @firebase/analytics

    Type Base (b07f822) Head (93d6d22) Diff
    esm2017 8.68 kB 9.89 kB +1.20 kB (+13.9%)
    main 9.69 kB 11.1 kB +1.42 kB (+14.7%)
    module 9.37 kB 10.8 kB +1.45 kB (+15.5%)
  • @firebase/auth

    Type Base (b07f822) Head (93d6d22) Diff
    browser ? 177 kB ? (?)
  • @firebase/database

    Type Base (b07f822) Head (93d6d22) Diff
    browser 268 kB 269 kB +849 B (+0.3%)
    esm2017 235 kB 236 kB +524 B (+0.2%)
    main 269 kB 270 kB +849 B (+0.3%)
    module 267 kB 268 kB +844 B (+0.3%)
  • @firebase/firestore

    Type Base (b07f822) Head (93d6d22) Diff
    browser 247 kB 249 kB +1.87 kB (+0.8%)
    esm2017 194 kB 195 kB +1.33 kB (+0.7%)
    main 495 kB 474 kB -20.8 kB (-4.2%)
    module 245 kB 246 kB +1.64 kB (+0.7%)
    react-native 194 kB 195 kB +1.33 kB (+0.7%)
  • @firebase/firestore/exp

    Type Base (b07f822) Head (93d6d22) Diff
    browser ? 189 kB ? (?)
    main 400 kB 467 kB +67.0 kB (+16.8%)
    module ? 189 kB ? (?)
    react-native ? 189 kB ? (?)
  • @firebase/firestore/lite

    Type Base (b07f822) Head (93d6d22) Diff
    browser ? 64.5 kB ? (?)
    main 124 kB 141 kB +17.2 kB (+13.9%)
    module ? 64.5 kB ? (?)
    react-native ? 64.6 kB ? (?)
  • @firebase/firestore/memory

    Type Base (b07f822) Head (93d6d22) Diff
    browser 185 kB 187 kB +2.24 kB (+1.2%)
    esm2017 145 kB 146 kB +1.30 kB (+0.9%)
    main 364 kB 348 kB -15.8 kB (-4.3%)
    module 183 kB 185 kB +2.04 kB (+1.1%)
    react-native 145 kB 146 kB +1.30 kB (+0.9%)
  • @firebase/messaging

    Type Base (b07f822) Head (93d6d22) Diff
    esm2017 23.2 kB 25.9 kB +2.68 kB (+11.5%)
    main 31.4 kB 34.6 kB +3.18 kB (+10.1%)
    module 30.9 kB 34.1 kB +3.17 kB (+10.2%)
  • @firebase/performance

    Type Base (b07f822) Head (93d6d22) Diff
    browser 26.7 kB 27.1 kB +376 B (+1.4%)
    esm2017 24.7 kB 25.1 kB +359 B (+1.5%)
    main 26.7 kB 27.1 kB +376 B (+1.4%)
    module 26.4 kB 26.8 kB +376 B (+1.4%)
  • @firebase/storage

    Type Base (b07f822) Head (93d6d22) Diff
    esm2017 56.4 kB 54.9 kB -1.42 kB (-2.5%)
    main 62.7 kB 61.3 kB -1.38 kB (-2.2%)
    module 62.5 kB 61.1 kB -1.39 kB (-2.2%)
  • @firebase/util

    Type Base (b07f822) Head (93d6d22) Diff
    browser 19.6 kB 20.5 kB +893 B (+4.6%)
    esm2017 17.5 kB 18.3 kB +758 B (+4.3%)
    main 19.6 kB 20.5 kB +893 B (+4.6%)
    module 18.7 kB 19.5 kB +804 B (+4.3%)
  • @firebase/webchannel-wrapper

    Type Base (b07f822) Head (93d6d22) Diff
    esm2017 37.7 kB 39.4 kB +1.69 kB (+4.5%)
    main 38.8 kB 41.0 kB +2.21 kB (+5.7%)
    module 38.4 kB 40.6 kB +2.21 kB (+5.8%)
  • firebase

    Click to show 15 binary size changes.
    Type Base (b07f822) Head (93d6d22) Diff
    firebase-analytics.js 26.6 kB 28.3 kB +1.72 kB (+6.5%)
    firebase-app.js 19.9 kB 20.0 kB +42 B (+0.2%)
    firebase-auth.js 174 kB 173 kB -184 B (-0.1%)
    firebase-database.js 187 kB 187 kB -486 B (-0.3%)
    firebase-firestore.js 285 kB 288 kB +2.30 kB (+0.8%)
    firebase-firestore.memory.js 224 kB 227 kB +3.05 kB (+1.4%)
    firebase-functions.js 9.84 kB 9.88 kB +43 B (+0.4%)
    firebase-installations.js 19.2 kB 19.2 kB +40 B (+0.2%)
    firebase-messaging.js 39.2 kB 40.9 kB +1.76 kB (+4.5%)
    firebase-performance-standalone.es2017.js 72.0 kB 72.1 kB +173 B (+0.2%)
    firebase-performance-standalone.js 47.4 kB 47.4 kB -24 B (-0.1%)
    firebase-performance.js 37.8 kB 37.8 kB -36 B (-0.1%)
    firebase-remote-config.js 37.0 kB 37.0 kB +40 B (+0.1%)
    firebase-storage.js 40.9 kB 39.9 kB -979 B (-2.4%)
    firebase.js 819 kB 823 kB +3.81 kB (+0.5%)

Test Logs

@changeset-bot
Copy link

changeset-bot bot commented Jun 28, 2020

🦋 Changeset is good to go

Latest commit: 374dc78

We got this.

This PR includes changesets to release 13 packages
Name Type
firebase Minor
@firebase/messaging
@firebase/messaging-types Minor
rxfire Major
@firebase/testing Patch
firebase-browserify-test Patch
firebase-package-typings-test Patch
firebase-messaging-selenium-test Patch
firebase-typescript-test Patch
firebase-webpack-test Patch
@firebase/functions Patch
@firebase/functions-exp Patch
firebase-exp Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@zwu52 zwu52 requested a review from kroikie June 29, 2020 18:26
Copy link

@kroikie kroikie left a comment

Choose a reason for hiding this comment

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

I would also recommend letting @egilmorez have a look at the comments.

integration/messaging/test/static/helpers.js Outdated Show resolved Hide resolved
packages/firebase/index.d.ts Outdated Show resolved Hide resolved
packages/firebase/index.d.ts Outdated Show resolved Hide resolved
packages/firebase/index.d.ts Outdated Show resolved Hide resolved
packages/firebase/index.d.ts Outdated Show resolved Hide resolved
packages/firebase/index.d.ts Show resolved Hide resolved
packages/firebase/index.d.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

Some wordsmithing things to look at Kai. Thanks!

packages/firebase/index.d.ts Outdated Show resolved Hide resolved
packages/firebase/index.d.ts Outdated Show resolved Hide resolved
packages/firebase/index.d.ts Outdated Show resolved Hide resolved
packages/firebase/index.d.ts Outdated Show resolved Hide resolved
packages/firebase/index.d.ts Outdated Show resolved Hide resolved
packages/firebase/index.d.ts Outdated Show resolved Hide resolved
packages/firebase/index.d.ts Outdated Show resolved Hide resolved
packages/firebase/index.d.ts Outdated Show resolved Hide resolved
packages/firebase/index.d.ts Outdated Show resolved Hide resolved
Copy link
Member

@Feiyang1 Feiyang1 left a comment

Choose a reason for hiding this comment

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

Added some comments. There is a bunch of formatting issues. Can you please run prettier on all files?

packages/firebase/index.d.ts Outdated Show resolved Hide resolved
packages/firebase/index.d.ts Outdated Show resolved Hide resolved
packages/firebase/index.d.ts Outdated Show resolved Hide resolved
packages/firebase/index.d.ts Outdated Show resolved Hide resolved
packages/firebase/index.d.ts Show resolved Hide resolved
packages/messaging/src/helpers/externalizePayload.ts Outdated Show resolved Hide resolved
@Feiyang1 Feiyang1 removed their assignment Jul 8, 2020
Copy link
Member Author

@zwu52 zwu52 left a comment

Choose a reason for hiding this comment

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

requested change will be included in the upcoming commit

packages/firebase/index.d.ts Show resolved Hide resolved
packages/firebase/index.d.ts Show resolved Hide resolved
packages/messaging/src/controllers/sw-controller.test.ts Outdated Show resolved Hide resolved
packages/messaging/src/helpers/externalizePayload.ts Outdated Show resolved Hide resolved
packages/messaging/src/controllers/sw-controller.ts Outdated Show resolved Hide resolved
packages/messaging/src/controllers/window-controller.ts Outdated Show resolved Hide resolved
TAG +
'no sw has been provided explicitly. Attempting to find firebase-messaging-sw.js in default directory.'
);
await this.registerDefaultSw();
Copy link
Member Author

Choose a reason for hiding this comment

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

absolutely

@zwu52 zwu52 requested review from Feiyang1 and egilmorez July 9, 2020 23:02
Copy link
Contributor

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

LG with a one-word nit, thanks!

packages/firebase/index.d.ts Outdated Show resolved Hide resolved
packages/installations/package.json Outdated Show resolved Hide resolved
packages/messaging/package.json Outdated Show resolved Hide resolved
.changeset/strange-crabs-tell.md Outdated Show resolved Hide resolved
packages/messaging/src/controllers/sw-controller.test.ts Outdated Show resolved Hide resolved
packages/messaging/src/controllers/sw-controller.ts Outdated Show resolved Hide resolved
packages/messaging/src/controllers/window-controller.ts Outdated Show resolved Hide resolved
@zwu52 zwu52 requested a review from Feiyang1 August 11, 2020 20:00
@zwu52 zwu52 merged commit 29327b2 into master Aug 11, 2020
@google-oss-bot google-oss-bot mentioned this pull request Aug 11, 2020
@firebase firebase locked and limited conversation to collaborators Sep 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants