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

Conversation

ehsannas
Copy link
Contributor

No description provided.

Copy link

changeset-bot bot commented Apr 19, 2024

🦋 Changeset detected

Latest commit: d46e014

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@firebase/firestore Patch
@firebase/webchannel-wrapper Major
firebase Patch
@firebase/firestore-compat Patch

Not sure what this means? Click here to learn what changesets are.

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

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Apr 19, 2024

Size Report 1

Affected Products

  • @firebase/firestore

    TypeBase (7709f10)Merge (ffd280a)Diff
    browser377 kB377 kB+69 B (+0.0%)
    esm5362 kB362 kB+69 B (+0.0%)
    main580 kB580 kB-53 B (-0.0%)
    module377 kB377 kB+69 B (+0.0%)
    react-native377 kB377 kB+69 B (+0.0%)
  • @firebase/webchannel-wrapper

    TypeBase (7709f10)Merge (ffd280a)Diff
    main65.4 kB38 B-65.4 kB (-99.9%)
  • @firebase/webchannel-wrapper/bloom-blob

    TypeBase (7709f10)Merge (ffd280a)Diff
    browser?11.1 kB? (?)
    esm5?11.1 kB? (?)
    main?11.1 kB? (?)
    module?11.1 kB? (?)
  • @firebase/webchannel-wrapper/webchannel-blob

    TypeBase (7709f10)Merge (ffd280a)Diff
    browser?44.3 kB? (?)
    esm5?55.6 kB? (?)
    main?44.3 kB? (?)
    module?44.3 kB? (?)
  • bundle

    TypeBase (7709f10)Merge (ffd280a)Diff
    firestore (CSI Auto Indexing Disable and Delete)269 kB270 kB+1.55 kB (+0.6%)
    firestore (CSI Auto Indexing Enable)269 kB270 kB+1.55 kB (+0.6%)
    firestore (Persistence)303 kB305 kB+1.56 kB (+0.5%)
    firestore (Query Cursors)241 kB242 kB+1.56 kB (+0.6%)
    firestore (Query)238 kB240 kB+1.56 kB (+0.7%)
    firestore (Read data once)226 kB228 kB+1.56 kB (+0.7%)
    firestore (Read Write w Persistence)323 kB324 kB+1.56 kB (+0.5%)
    firestore (Realtime updates)229 kB230 kB+1.56 kB (+0.7%)
    firestore (Transaction)205 kB207 kB+1.55 kB (+0.8%)
    firestore (Write data)205 kB207 kB+1.55 kB (+0.8%)
  • firebase

    TypeBase (7709f10)Merge (ffd280a)Diff
    firebase-compat.js786 kB787 kB+1.55 kB (+0.2%)
    firebase-firestore-compat.js342 kB343 kB+1.10 kB (+0.3%)
    firebase-firestore.js436 kB438 kB+1.54 kB (+0.4%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/P92GX4HZLu.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Apr 22, 2024

Size Analysis Report 1

This report is too large (179,942 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/KQRehKwmhI.html

@ehsannas ehsannas marked this pull request as ready for review April 22, 2024 23:02
@ehsannas ehsannas requested review from a team as code owners April 22, 2024 23:02
@ehsannas ehsannas self-assigned this Apr 22, 2024
Copy link
Contributor

github-actions bot commented Apr 26, 2024

Changeset File Check ✅

  • No modified packages are missing from the changeset file.
  • No changeset formatting errors detected.

@ehsannas ehsannas requested a review from a team as a code owner April 26, 2024 22:09
Copy link

@12wrigja 12wrigja left a comment

Choose a reason for hiding this comment

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

This seems fine to me w.r.t. the dep on closure-net, but I'll leave actual approval up to firebase team members.

Copy link
Contributor

@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

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

LGTM

* "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.

Copy link
Contributor

@MarkDuckworth MarkDuckworth left a comment

Choose a reason for hiding this comment

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

LGTM, with exception of the versioning of webchannel-wrapper.

@ehsannas ehsannas merged commit 4b49630 into master May 1, 2024
44 checks passed
@ehsannas ehsannas deleted the ehsann/closure-net branch May 1, 2024 19:38
@google-oss-bot google-oss-bot mentioned this pull request May 8, 2024
@firebase firebase locked and limited conversation to collaborators Jun 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants