The Wayback Machine - https://web.archive.org/web/20211202112943/https://github.com/firebase/firebase-js-sdk/pull/3822
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

Jump to bottom

Create experiment modularized FM SDK #3822

Merged
merged 18 commits into from Dec 21, 2020
Merged

Create experiment modularized FM SDK #3822

merged 18 commits into from Dec 21, 2020

Conversation

@zwu52
Copy link
Member

@zwu52 zwu52 commented Sep 21, 2020 •
edited
Loading

FM Modularization. Part of go/firebase-next

Dependency updated: app -> app-exp, app-type -> app-types-exp, messaging-types -> messaging-types-exp
Dependency to-be-updates: firebase-installations (there is some issue with stubbing getToken(installations) call. Update dependency to firebase-installtions-exp in proceeding PR
@zwu52 zwu52 requested review from Feiyang1 and hsubox76 Sep 21, 2020
@zwu52 zwu52 requested review from hiranya911 and as code owners Sep 21, 2020
@changeset-bot
Copy link

@changeset-bot changeset-bot bot commented Sep 21, 2020 •
edited
Loading

⚠️ No Changeset found

Latest commit: 9847c41

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

💥 An error occurred when fetching the changed packages and changesets in this PR 
Some errors occurred when validating the changesets config:
The package "@firebase/messaging-exp" depends on the ignored package "@firebase/app-exp", but "@firebase/messaging-exp" is not being ignored. Please add "@firebase/messaging-exp" to the `ignore` option.
The package "@firebase/messaging-exp" depends on the ignored package "@firebase/app-types-exp", but "@firebase/messaging-exp" is not being ignored. Please add "@firebase/messaging-exp" to the `ignore` option.
The package "@firebase/messaging-exp" depends on the ignored package "@firebase/installations-exp", but "@firebase/messaging-exp" is not being ignored. Please add "@firebase/messaging-exp" to the `ignore` option.

Loading

@zwu52 zwu52 marked this pull request as draft Sep 21, 2020
@google-oss-bot
Copy link
Contributor

@google-oss-bot google-oss-bot commented Sep 21, 2020 •
edited
Loading

Binary Size Report

Affected SDKs

  • @firebase/firestore

    Type Base (5593be6) Head (161011e) Diff
    browser 249 kB 249 kB +441 B (+0.2%)
    esm2017 196 kB 196 kB +201 B (+0.1%)
    main 483 kB 483 kB +624 B (+0.1%)
    module 246 kB 247 kB +451 B (+0.2%)
    react-native 196 kB 196 kB +201 B (+0.1%)

  • @firebase/firestore/exp

    Type Base (5593be6) Head (161011e) Diff
    browser 189 kB 189 kB -428 B (-0.2%)
    main 478 kB 477 kB -909 B (-0.2%)
    module 189 kB 189 kB -428 B (-0.2%)
    react-native 189 kB 189 kB -428 B (-0.2%)

  • @firebase/firestore/lite

    Type Base (5593be6) Head (161011e) Diff
    browser 63.8 kB 63.4 kB -436 B (-0.7%)
    main 141 kB 140 kB -963 B (-0.7%)
    module 63.8 kB 63.4 kB -436 B (-0.7%)
    react-native 64.1 kB 63.6 kB -436 B (-0.7%)

  • @firebase/firestore/memory

    Type Base (5593be6) Head (161011e) Diff
    browser 186 kB 187 kB +433 B (+0.2%)
    esm2017 147 kB 147 kB +193 B (+0.1%)
    main 356 kB 357 kB +570 B (+0.2%)
    module 184 kB 185 kB +443 B (+0.2%)
    react-native 147 kB 147 kB +193 B (+0.1%)

  • @firebase/functions

    Type Base (5593be6) Head (161011e) Diff
    browser 9.77 kB 10.0 kB +263 B (+2.7%)
    esm2017 7.35 kB 7.59 kB +238 B (+3.2%)
    main 9.89 kB 10.2 kB +263 B (+2.7%)
    module 9.50 kB 9.77 kB +263 B (+2.8%)

  • firebase

    Type Base (5593be6) Head (161011e) Diff
    firebase-firestore.js 287 kB 287 kB +447 B (+0.2%)
    firebase-firestore.memory.js 226 kB 227 kB +439 B (+0.2%)
    firebase-functions.js 9.94 kB 10.1 kB +160 B (+1.6%)
    firebase.js 830 kB 831 kB +606 B (+0.1%)

Test Logs

Loading

@google-oss-bot

This comment has been hidden.

Sign in to view
@zwu52 zwu52 marked this pull request as ready for review Sep 25, 2020
@Feiyang1 Feiyang1 self-assigned this Oct 12, 2020
Feiyang1
Feiyang1 reviewed Oct 17, 2020
View changes
Copy link
Member

@Feiyang1 Feiyang1 left a comment

The API layer looks good, but we didn't really modularize the SDK as we still implements the methods on the controller.

We need to move these methods to free floating functions in order to make them tree shakeable. I also think we should have a separate entry point for service worker, so we don't force people to include the service worker code in their client code.

Loading

packages-exp/messaging-exp/.npmignore Outdated Show resolved Hide resolved
Loading
packages-exp/messaging-exp/package.json Outdated Show resolved Hide resolved
Loading
packages-exp/messaging-exp/package.json Outdated Show resolved Hide resolved
Loading
@Feiyang1 Feiyang1 assigned zwu52 and unassigned Feiyang1 Oct 22, 2020
@zwu52 zwu52 requested a review from Feiyang1 Oct 29, 2020
@github-actions
Copy link
Contributor

@github-actions github-actions bot commented Nov 17, 2020 •
edited
Loading

Changeset File Check ⚠️

  • Changeset formatting error in following file: 
    Package "@firebase/messaging-exp" must depend on the current version of "@firebase/util": "0.3.4" vs "0.3.2"
{ ValidationError: Some errors occurred when validating the changesets config:
The package "@firebase/messaging-exp" depends on the ignored package "@firebase/app-exp", but "@firebase/messaging-exp" is not being ignored. Please add "@firebase/messaging-exp" to the `ignore` option.
The package "@firebase/messaging-exp" depends on the ignored package "@firebase/app-types-exp", but "@firebase/messaging-exp" is not being ignored. Please add "@firebase/messaging-exp" to the `ignore` option.
The package "@firebase/messaging-exp" depends on the ignored package "@firebase/installations-exp", but "@firebase/messaging-exp" is not being ignored. Please add "@firebase/messaging-exp" to the `ignore` option.
  name: 'ValidationError',
  _error:
   Error
       at new ExtendableError (/home/runner/work/firebase-js-sdk/firebase-js-sdk/node_modules/extendable-error/bld/index.js:23:24)
       at new ValidationError (/home/runner/work/firebase-js-sdk/firebase-js-sdk/node_modules/@changesets/errors/dist/errors.cjs.dev.js:16:1)
       at parse (/home/runner/work/firebase-js-sdk/firebase-js-sdk/node_modules/@changesets/cli/node_modules/@changesets/config/dist/config.cjs.dev.js:196:11)
       at Object.read (/home/runner/work/firebase-js-sdk/firebase-js-sdk/node_modules/@changesets/cli/node_modules/@changesets/config/dist/config.cjs.dev.js:84:10) }

Loading

@hsubox76
Copy link
Contributor

@hsubox76 hsubox76 commented Nov 18, 2020

FYI to avoid that changeset error you will want to add @firebase/messaging-exp to the ignore array in .changeset/config.json.

Loading

Feiyang1
Feiyang1 reviewed Dec 1, 2020
View changes
Copy link
Member

@Feiyang1 Feiyang1 left a comment

Overall it looks great! Left a few comments and I'd like to do a size analysis on the new APIs to see how treeshaking works once PR is merged.

Loading

.vscode/settings.json Outdated Show resolved Hide resolved
Loading
packages-exp/messaging-exp/package.json Outdated Show resolved Hide resolved
Loading
packages-exp/messaging-exp/package.json Outdated
"dependencies": {
"@firebase/component": "0.1.19",
"@firebase/installations-exp": "0.x",
"@firebase/messaging-types-exp": "0.0.800",
Copy link
Member

@Feiyang1 Feiyang1 Dec 1, 2020

Choose a reason for hiding this comment

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

Suggested change
"@firebase/messaging-types-exp": "0.0.800",
"@firebase/messaging-types-exp": "0.0.900",

Loading

packages-exp/messaging-exp/package.json Show resolved Hide resolved
Loading
packages-exp/messaging-exp/src/index.ts Outdated Show resolved Hide resolved
Loading
packages-exp/messaging-exp/src/index.ts Outdated Show resolved Hide resolved
Loading
packages-exp/messaging-exp/src/interfaces/internal-dependencies.ts Outdated Show resolved Hide resolved
Loading
packages-exp/messaging-types-exp/package.json Outdated Show resolved Hide resolved
Loading
packages-exp/messaging-exp/src/api/onBackgroundMessage.ts Show resolved Hide resolved
Loading
packages-exp/messaging-exp/src/index.ts Show resolved Hide resolved
Loading
@Feiyang1
Copy link
Member

@Feiyang1 Feiyang1 commented Dec 4, 2020

@zwu52 ping

Loading

@zwu52 zwu52 requested a review from Feiyang1 Dec 15, 2020
hsubox76
hsubox76 reviewed Dec 15, 2020
View changes
Copy link
Contributor

@hsubox76 hsubox76 left a comment

I see a couple of unaddressed comments by Fei (or maybe I am looking at Github UI wrong). In addition to that I added one or two comments.

Also, you don't have to create a changeset for exp, but in order to stop the changeset tool from erroring when we run releases, you have to add the package to the ignore field in the changeset config: see comment #3822 (comment) and the error above it.

Loading

packages-exp/messaging-exp/src/helpers/register.ts Show resolved Hide resolved
Loading
common/api-review/messaging-exp.api.md Outdated
import { Observer } from '@firebase/util';
import { Unsubscribe } from '@firebase/util';
// @public (undocumented)
Copy link
Contributor

@hsubox76 hsubox76 Dec 15, 2020

Choose a reason for hiding this comment

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

Maybe you are planning to do this in another PR but to get documentation, the JSdoc comments need to be added in src/api.ts

Loading

Feiyang1
Feiyang1 reviewed Dec 16, 2020
View changes
Copy link
Member

@Feiyang1 Feiyang1 left a comment •
edited
Loading

Looks great. I think we are almost ready. We need to create a release build for publishing to npm. You can take a look at https://github.com/firebase/firebase-js-sdk/blob/master/packages-exp/functions-exp/rollup.config.release.js. What it does is remove -exps in the code, so the package is published as @firebase/messaging instead of @firebase/messaging-exp (it will be published under a special tag, so it won't interfere with the normal release).

We also need to add messaging to firebase-exp, so people can import from firebase/messaging.

In addition to that, we need to build 2 CDN scripts for messaging in firebase-exp, one for the client script, one for the service worker script. We can do it in a separate PR though. Let me know if you need help.

FYI, @hsubox76 - we need to make sure the service worker script is included in our distribution, otherwise people won't be able to use messaging.

Loading

packages-exp/messaging-exp/src/messaging-service.ts Outdated Show resolved Hide resolved
Loading
@zwu52
Copy link
Member Author

@zwu52 zwu52 commented Dec 17, 2020

Thanks Fei & Christina for reviewing.
I created a release rollup config.

in regards to "We also need to add messaging to firebase-exp, so people can import from firebase/messaging." Looks like FM is already being imported in the constants.ts or is there another place to import?

Loading

@Feiyang1
Copy link
Member

@Feiyang1 Feiyang1 commented Dec 17, 2020

I meant that we need to make messaging-exp available in the umbrella package firebase-exp by adding a folder for messaging here - https://github.com/firebase/firebase-js-sdk/tree/master/packages-exp/firebase-exp

Loading

zwu52 added 6 commits Dec 17, 2020
@zwu52
Add messaging to firebase-exp/
69d9d1b
@zwu52
Fix typing error
3fdb6d9
@zwu52
Try fix type error
370575c 
previously working. start showing errror after the register.ts refactor. reverting back to see
@zwu52
Rmove moduale declartion
0028246
@zwu52
fix
f41f23a
@zwu52
Remove unused deps
96ceea6
@zwu52 zwu52 requested review from Feiyang1 and hsubox76 Dec 17, 2020
@zwu52
Copy link
Member Author

@zwu52 zwu52 commented Dec 17, 2020 •
edited
Loading

Tried a bunch of fix for

Error: /home/runner/work/firebase-js-sdk/firebase-js-sdk/packages-exp/messaging-exp/src/helpers/register.ts(38,27): semantic error TS2345: Argument of type '"app-exp"' is not assignable to parameter of type '"messaging-exp"'.

I am guessing I messed up something but not sure how. has this been seen previously?

Loading

@Feiyang1
Copy link
Member

@Feiyang1 Feiyang1 commented Dec 18, 2020

Make sure that the version of dependency @firebase/component is the same as packages/component. I got a different error when compiling, but it went away once I updated the version.

Loading

Feiyang1
Feiyang1 reviewed Dec 18, 2020
View changes
packages-exp/firebase-exp/package.json Show resolved Hide resolved
Loading
packages-exp/messaging-exp/package.json Outdated Show resolved Hide resolved
Loading
packages-exp/messaging-exp/package.json Outdated Show resolved Hide resolved
Loading
@zwu52 zwu52 requested a review from Feiyang1 Dec 21, 2020
Feiyang1
Feiyang1 approved these changes Dec 21, 2020
View changes
Copy link
Member

@Feiyang1 Feiyang1 left a comment

LGTM. One todo which we can do in a separate PR - we need to build the CDN script the service worker in firebase-exp, currently we only build a client CDN script for messaging.

Loading

@zwu52 zwu52 merged commit e13b794 into master Dec 21, 2020
15 of 16 checks passed
Loading
@zwu52
Copy link
Member Author

@zwu52 zwu52 commented Dec 21, 2020

will do

Loading

@zwu52
Copy link
Member Author

@zwu52 zwu52 commented Dec 23, 2020

@hsubox76 @Feiyang1
Trying to create the CDN build and need some input here. I think what I am targeting is to create a index.cdn.ts under pacakges-exp/firebase-exp/messaging/ and add a build rule in firebase-exp/rollup.config.js like that of app's (CMIIW). I am trying to have

export { onBackgroudMessage } from '@firebase/messaging-exp';

for index.cdn.ts however @firebase/messaging-exp points to messaging-exp/dist/index.d.ts. What am I missing to have the file to point to messaging-exp/dist/index.sw.d.ts? (or am I working with the wrong materials).

Loading

@firebase firebase locked and limited conversation to collaborators Jan 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Feiyang1 Feiyang1

@hiranya911 hiranya911

@hsubox76 hsubox76

Assignees

@zwu52 zwu52

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@zwu52 @google-oss-bot @hsubox76 @Feiyang1