Create experiment modularized FM SDK #3822

zwu52 · 2020-09-21T22:06:59Z

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

changeset-bot · 2020-09-21T22:07:02Z

⚠️ 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.

google-oss-bot · 2020-09-21T22:23:33Z

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

Base (5593be66): https://github.com/firebase/firebase-js-sdk/actions/runs/265816220
Head (161011ec): https://github.com/firebase/firebase-js-sdk/actions/runs/277117101

Feiyang1

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.

packages-exp/messaging-exp/.npmignore

packages-exp/messaging-exp/package.json

github-actions · 2020-11-17T00:36:12Z

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) }

hsubox76 · 2020-11-18T18:10:45Z

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

Feiyang1

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.

.vscode/settings.json

packages-exp/messaging-exp/package.json

Feiyang1 · 2020-12-01T19:00:16Z

packages-exp/messaging-exp/package.json

+  "dependencies": {
+    "@firebase/component": "0.1.19",
+    "@firebase/installations-exp": "0.x",
+    "@firebase/messaging-types-exp": "0.0.800",


Suggested change

"@firebase/messaging-types-exp": "0.0.800",

"@firebase/messaging-types-exp": "0.0.900",

packages-exp/messaging-exp/package.json

packages-exp/messaging-exp/src/index.ts

packages-exp/messaging-exp/src/interfaces/internal-dependencies.ts

packages-exp/messaging-types-exp/package.json

packages-exp/messaging-exp/src/api/onBackgroundMessage.ts

packages-exp/messaging-exp/src/index.ts

Feiyang1 · 2020-12-04T23:26:31Z

@zwu52 ping

…into mod

hsubox76

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.

packages-exp/messaging-exp/src/helpers/register.ts

hsubox76 · 2020-12-15T22:08:58Z

common/api-review/messaging-exp.api.md

+import { Observer } from '@firebase/util';
+import { Unsubscribe } from '@firebase/util';
+
+// @public (undocumented)


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

Feiyang1

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.

packages-exp/messaging-exp/src/messaging-service.ts

-

zwu52 · 2020-12-17T01:05:22Z

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?

Feiyang1 · 2020-12-17T18:19:29Z

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

previously working. start showing errror after the register.ts refactor. reverting back to see

zwu52 · 2020-12-17T21:47:32Z

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?

Feiyang1 · 2020-12-18T18:46:38Z

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.

packages-exp/firebase-exp/package.json

packages-exp/messaging-exp/package.json

Feiyang1

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.

zwu52 · 2020-12-21T18:52:23Z

will do

zwu52 · 2020-12-23T01:13:45Z

@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).

FM Modualization

6a4a669

zwu52 requested review from Feiyang1 and hsubox76 September 21, 2020 22:06

zwu52 requested review from hiranya911 and a team as code owners September 21, 2020 22:06

zwu52 marked this pull request as draft September 21, 2020 22:17

zwu52 and others added 3 commits September 22, 2020 14:12

Update dependecies to exp

31fa5a1

update app dependency

6b04715

Update README.md

d777e33

This comment has been minimized.

Sign in to view

zwu52 marked this pull request as ready for review September 25, 2020 21:21

Integrate w/ API checker

de78923

Feiyang1 self-assigned this Oct 12, 2020

Feiyang1 reviewed Oct 17, 2020

View reviewed changes

packages-exp/messaging-exp/.npmignore Outdated Show resolved Hide resolved

packages-exp/messaging-exp/package.json Outdated Show resolved Hide resolved

packages-exp/messaging-exp/package.json Outdated Show resolved Hide resolved

Feiyang1 assigned zwu52 and unassigned Feiyang1 Oct 22, 2020

Removing deps that are already included in the top level

724bba9

zwu52 requested a review from Feiyang1 October 29, 2020 17:31

Mod Change

1c49b44

Feiyang1 reviewed Dec 1, 2020

View reviewed changes

zwu52 added 2 commits December 9, 2020 13:38

Merge branch 'master' of https://github.com/firebase/firebase-js-sdk …

36e7586

…into mod

Update

3d8a351

zwu52 requested a review from Feiyang1 December 15, 2020 21:47

hsubox76 reviewed Dec 15, 2020

View reviewed changes

Add js doc

e4b967f

Feiyang1 reviewed Dec 16, 2020

View reviewed changes

packages-exp/messaging-exp/src/messaging-service.ts Outdated Show resolved Hide resolved

Create release roll up

4308cd3

-

zwu52 added 6 commits December 17, 2020 10:49

Add messaging to firebase-exp/

69d9d1b

Fix typing error

3fdb6d9

Try fix type error

370575c

previously working. start showing errror after the register.ts refactor. reverting back to see

Rmove moduale declartion

0028246

fix

f41f23a

Remove unused deps

96ceea6

zwu52 requested review from Feiyang1 and hsubox76 December 17, 2020 21:43

Feiyang1 reviewed Dec 18, 2020

View reviewed changes

packages-exp/firebase-exp/package.json Show resolved Hide resolved

packages-exp/messaging-exp/package.json Outdated Show resolved Hide resolved

packages-exp/messaging-exp/package.json Outdated Show resolved Hide resolved

updates dep versions

9847c41

zwu52 requested a review from Feiyang1 December 21, 2020 18:28

Feiyang1 approved these changes Dec 21, 2020

View reviewed changes

zwu52 merged commit e13b794 into master Dec 21, 2020

firebase locked and limited conversation to collaborators Jan 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create experiment modularized FM SDK #3822

Create experiment modularized FM SDK #3822

zwu52 commented Sep 21, 2020 •

edited

Loading

changeset-bot bot commented Sep 21, 2020 •

edited

Loading

google-oss-bot commented Sep 21, 2020 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore/exp`

`@firebase/firestore/lite`

`@firebase/firestore/memory`

`@firebase/functions`

`firebase`

This comment has been minimized.

Feiyang1 left a comment

github-actions bot commented Nov 17, 2020 •

edited

Loading

hsubox76 commented Nov 18, 2020

Feiyang1 left a comment

Feiyang1 Dec 1, 2020

Feiyang1 commented Dec 4, 2020

hsubox76 left a comment

hsubox76 Dec 15, 2020

Feiyang1 left a comment •

edited

Loading

zwu52 commented Dec 17, 2020

Feiyang1 commented Dec 17, 2020

zwu52 commented Dec 17, 2020 •

edited

Loading

Feiyang1 commented Dec 18, 2020

Feiyang1 left a comment

zwu52 commented Dec 21, 2020

zwu52 commented Dec 23, 2020

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

Create experiment modularized FM SDK #3822

Create experiment modularized FM SDK #3822

Conversation

zwu52 commented Sep 21, 2020 • edited Loading

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

⚠️ No Changeset found

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

Binary Size Report

Affected SDKs

@firebase/firestore

@firebase/firestore/exp

@firebase/firestore/lite

@firebase/firestore/memory

@firebase/functions

firebase

Test Logs

This comment has been minimized.

Feiyang1 left a comment

Choose a reason for hiding this comment

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

Changeset File Check ⚠️

hsubox76 commented Nov 18, 2020

Feiyang1 left a comment

Choose a reason for hiding this comment

Feiyang1 Dec 1, 2020

Choose a reason for hiding this comment

Feiyang1 commented Dec 4, 2020

hsubox76 left a comment

Choose a reason for hiding this comment

hsubox76 Dec 15, 2020

Choose a reason for hiding this comment

Feiyang1 left a comment • edited Loading

Choose a reason for hiding this comment

zwu52 commented Dec 17, 2020

Feiyang1 commented Dec 17, 2020

zwu52 commented Dec 17, 2020 • edited Loading

Feiyang1 commented Dec 18, 2020

Feiyang1 left a comment

Choose a reason for hiding this comment

zwu52 commented Dec 21, 2020

zwu52 commented Dec 23, 2020

zwu52 commented Sep 21, 2020 •

edited

Loading

changeset-bot bot commented Sep 21, 2020 •

edited

Loading

google-oss-bot commented Sep 21, 2020 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore/exp`

`@firebase/firestore/lite`

`@firebase/firestore/memory`

`@firebase/functions`

`firebase`

github-actions bot commented Nov 17, 2020 •

edited

Loading

Feiyang1 left a comment •

edited

Loading

zwu52 commented Dec 17, 2020 •

edited

Loading