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.

Sign up for GitHub

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

Use closure-net as a dependency of Firestore #8190

Merged
merged 17 commits into from
May 1, 2024
Merged

Use closure-net as a dependency of Firestore #8190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
6975f8a
Add closure library blobs as Firestore dependency.
ehsannas Apr 19, 2024
c514a7a
Remove our compilation of Closure.
ehsannas Apr 19, 2024
796b4db
Add closure-net as a git submodule.
ehsannas Apr 19, 2024
cd7ef78
remove temporary .only in test.
ehsannas Apr 19, 2024
06a6ea1
prettier.
ehsannas Apr 19, 2024
5297574
add yarn.lock differences.
ehsannas Apr 19, 2024
ce715d6
create a types.d.ts for the webchannel-wrapper package.
ehsannas Apr 19, 2024
d95ded5
Bring back the aggregated types and implementations for size report.
ehsannas Apr 22, 2024
433aee1
Undo CI changes for git submodules.
ehsannas Apr 23, 2024
4d11e1c
Remove the git submodule.
ehsannas Apr 23, 2024
7c2f071
Use git+https in package.json devDependencies.
ehsannas Apr 23, 2024
f3bc8f5
prettier.
ehsannas Apr 23, 2024
6834524
Set up rollup for webchannel-wrapper (#8203)
hsubox76 Apr 26, 2024
6b3edf8
Merge remote-tracking branch 'origin/master' into closure-net
ehsannas Apr 26, 2024
726fc7c
Create thick-spoons-check.md
ehsannas Apr 26, 2024
1a2c458
Better changeset.
ehsannas Apr 26, 2024
d46e014
Address feedback on versioning.
ehsannas Apr 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/thick-spoons-check.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@firebase/firestore": patch
"@firebase/webchannel-wrapper": major
---

Use closure-net as a dependency of webchannel-wrapper and Firestore.
Empty file added .gitmodules
View file
Open in desktop
Empty file.
3 changes: 2 additions & 1 deletion packages/firestore/externs.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@
"packages/component/dist/src/provider.d.ts",
"packages/component/dist/src/component_container.d.ts",
"packages/logger/dist/src/logger.d.ts",
"packages/webchannel-wrapper/src/index.d.ts",
"packages/webchannel-wrapper/dist/bloom-blob/bloom_blob_types.d.ts",
"packages/webchannel-wrapper/dist/webchannel-blob/webchannel_blob_types.d.ts",
"packages/util/dist/src/crypt.d.ts",
"packages/util/dist/src/defaults.d.ts",
"packages/util/dist/src/emulator.d.ts",
Expand Down
6 changes: 3 additions & 3 deletions packages/firestore/src/platform/browser/webchannel_connection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ import {
EventTarget,
StatEvent,
Event,
Stat,
FetchXmlHttpFactory
} from '@firebase/webchannel-wrapper';
FetchXmlHttpFactory,
Stat
} from '@firebase/webchannel-wrapper/webchannel-blob';
MarkDuckworth marked this conversation as resolved.
Show resolved Hide resolved

import { Token } from '../../api/credentials';
import { ExperimentalLongPollingOptions } from '../../api/long_polling_options';
Expand Down
3 changes: 2 additions & 1 deletion packages/firestore/src/remote/bloom_filter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import { Md5, Integer } from '@firebase/webchannel-wrapper';

import { Md5, Integer } from '@firebase/webchannel-wrapper/bloom-blob';

import { newTextEncoder } from '../platform/text_serializer';

Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/test/unit/core/webchannel_wrapper.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
// These tests are mostly to ensure that the exported classes correctly map to
// underlying functionality from google-closure-library.

import { Md5, Integer } from '@firebase/webchannel-wrapper';
import { Md5, Integer } from '@firebase/webchannel-wrapper/bloom-blob';
import { expect } from 'chai';

import { newTextEncoder } from '../../../src/platform/text_serializer';
Expand Down
9 changes: 9 additions & 0 deletions packages/webchannel-wrapper/bloom-blob/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"name": "@firebase/webchannel-wrapper/bloom-blob",
"description": "Bloom filter related code from the Closure library.",
"main": "../dist/bloom-blob/bloom_blob_es2018.js",
"browser": "../dist/bloom-blob/esm/bloom_blob_es2018.js",
"module": "../dist/bloom-blob/esm/bloom_blob_es2018.js",
"esm5": "../dist/bloom-blob/bloom_blob_es5.js",
"typings": "../dist/bloom-blob/bloom_blob_types.d.ts"
}
11 changes: 8 additions & 3 deletions ...ages/webchannel-wrapper/externs/module.js → packages/webchannel-wrapper/empty.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2020 Google LLC
* Copyright 2024 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -15,5 +15,10 @@
* limitations under the License.
*/

/** @type {!Object} */
const module = {};
/**
* This package has no main entry point and only allows imports from its
* two subpaths. This file is provided for the top-level package.json
* "main" field to point to.
*/

export default {};
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume you considered this. Why can't this file re-export FetchXmlHttpFactory, Stat, MD5, and Integer, so the imports don't have to change?

Copy link
Contributor

@hsubox76 hsubox76 Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I remember the rationale was that one package only need bloom-blob and another package needed both, so this would allow the consumer that only needs bloom to avoid bringing in webchannel code, although I guess with the correct build tool config, you can tree shake from the same package. I don't remember what the 2 consumers were, I guess browser vs non-browser bundles?

Edit: I don't think you can tree-shake in a way to separate individual object within the bloom-blob or webchannel-blob from each other since they're minified and all wrapped in an IIFE? But you should be able to bring in only bloom-blob code or only webchannel-wrapper code since they're separate files entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1. the thinking was that one package (i.e. firestore) needs both blobs whereas another package (another firebase product) will only need the webchannel blob. If tree-shaking can't be done then this is the cleaner way to go.

Copy link

Choose a reason for hiding this comment

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

Are you able to rely on your customers having tree-shaking configured (or even any particular bundler setup)?

When I prototyped this, I actually made an entirely new local package for the bloom blobs thinking this would be the only way to avoid shipping the bloom code unnecessarily. It might even be feasible to ask your build tools to integrate that local package into the relevant firebase packages (instead of having another package to publish to the registry that is supposed to be firebase-internal).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why we weren't bundling webchannel-wrapper code to begin with but I think it's worth exploring if we can bundle it in the future, we should make a note to look into this later. I think it's fine to roll with keeping the separate package for now until we are able to fully explore any possible problems with bundling.

86 changes: 0 additions & 86 deletions packages/webchannel-wrapper/externs/overrides.js
View file
Open in desktop

This file was deleted.

218 changes: 0 additions & 218 deletions packages/webchannel-wrapper/gulpfile.js
View file
Open in desktop

This file was deleted.

Loading
Loading