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

Use closure-net as a dependency of Firestore #8190

Merged
merged 17 commits into from
May 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
Empty file.
3 changes: 2 additions & 1 deletion packages/firestore/externs.json
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
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
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
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
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"
}
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

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.

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

This file was deleted.

218 changes: 0 additions & 218 deletions packages/webchannel-wrapper/gulpfile.js

This file was deleted.

Loading
Loading