-
Notifications
You must be signed in to change notification settings - Fork 943
Use closure-net as a dependency of Firestore #8190
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
Changes from all commits
6975f8a
c514a7a
796b4db
cd7ef78
06a6ea1
5297574
ce715d6
d95ded5
433aee1
4d11e1c
7c2f071
f3bc8f5
6834524
6b3edf8
726fc7c
1a2c458
d46e014
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. |
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. | ||
|
@@ -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 {}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
This file was deleted.
This file was deleted.
Uh oh!
There was an error while loading. Please reload this page.