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

Migrate from the Node to the Web ReadableStream #8410

Merged
merged 7 commits into from
Aug 14, 2024
Merged

Migrate from the Node to the Web ReadableStream #8410

merged 7 commits into from
Aug 14, 2024
+66 −47

Conversation

dlarocque
Copy link
Contributor

@dlarocque dlarocque commented Aug 6, 2024 •
edited
Loading

In the past, we used node-fetch, which used the NodeJS.ReadableStream interface for streaming requests. When we moved to undici (#7705), we began using the Web ReadableStream interface under the hood, but did not update our usage of the interface, so users began seeing errors when using getStream (#8303).

To fix this issue, we need to migrate from the Node to the Web ReadableStream interface.

This will help us in the future when we migrate to native Node fetch, since that also uses the Web ReadableStream interface.

Fixes #8303

@dlarocque dlarocque requested review from a team and tonyjhuang as code owners August 6, 2024 21:17
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Aug 6, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 85a432a

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

This PR includes changesets to release 3 packages
Name Type
firebase Minor
@firebase/storage Minor
@firebase/storage-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

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Aug 6, 2024 •
edited
Loading

Changeset File Check ✅

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

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Aug 6, 2024 •
edited
Loading

Size Report 1

Affected Products

  • @firebase/firestore

    TypeBase (1601572)Merge (6e4609c)Diff
    browser379 kB381 kB+2.16 kB (+0.6%)
    esm5364 kB366 kB+1.94 kB (+0.5%)
    main584 kB587 kB+3.37 kB (+0.6%)
    module379 kB381 kB+2.16 kB (+0.6%)
    react-native379 kB382 kB+2.16 kB (+0.6%)

  • @firebase/firestore-lite

    TypeBase (1601572)Merge (6e4609c)Diff
    browser109 kB111 kB+1.70 kB (+1.6%)
    esm5106 kB108 kB+1.56 kB (+1.5%)
    main150 kB153 kB+2.55 kB (+1.7%)
    module109 kB111 kB+1.70 kB (+1.6%)
    react-native109 kB111 kB+1.70 kB (+1.6%)

  • @firebase/storage

    TypeBase (1601572)Merge (6e4609c)Diff
    main59.4 kB59.4 kB-7 B (-0.0%)

  • bundle

    15 size changes

    TypeBase (1601572)Merge (6e4609c)Diff
    firestore (CSI Auto Indexing Disable and Delete)269 kB270 kB+934 B (+0.3%)
    firestore (CSI Auto Indexing Enable)269 kB270 kB+934 B (+0.3%)
    firestore (Persistence)304 kB305 kB+934 B (+0.3%)
    firestore (Query Cursors)241 kB242 kB+1.38 kB (+0.6%)
    firestore (Query)238 kB240 kB+1.38 kB (+0.6%)
    firestore (Read data once)226 kB228 kB+1.09 kB (+0.5%)
    firestore (Read Write w Persistence)323 kB325 kB+1.74 kB (+0.5%)
    firestore (Realtime updates)229 kB230 kB+1.09 kB (+0.5%)
    firestore (Transaction)205 kB207 kB+1.38 kB (+0.7%)
    firestore (Write data)205 kB207 kB+1.12 kB (+0.5%)
    firestore-lite (Query Cursors)89.8 kB91.2 kB+1.38 kB (+1.5%)
    firestore-lite (Query)85.9 kB87.3 kB+1.38 kB (+1.6%)
    firestore-lite (Read data once)62.1 kB62.8 kB+702 B (+1.1%)
    firestore-lite (Transaction)87.1 kB88.1 kB+995 B (+1.1%)
    firestore-lite (Write data)71.7 kB72.4 kB+730 B (+1.0%)

  • firebase

    TypeBase (1601572)Merge (6e4609c)Diff
    firebase-compat.js786 kB788 kB+1.65 kB (+0.2%)
    firebase-firestore-compat.js342 kB344 kB+1.65 kB (+0.5%)
    firebase-firestore-lite.js117 kB119 kB+1.69 kB (+1.4%)
    firebase-firestore.js438 kB440 kB+2.15 kB (+0.5%)

Test Logs

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

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Aug 6, 2024 •
edited
Loading

Size Analysis Report 1

This report is too large (631,171 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/6JHIJFgmS1.html

hsubox76
hsubox76 reviewed Aug 8, 2024
View reviewed changes
Copy link
Contributor

@hsubox76 hsubox76 left a comment

Choose a reason for hiding this comment

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

This is looking good, just have some questions on the new TransformStream syntax maybe just because I am unfamiliar with it. Maybe we can walk through quickly offline.

packages/storage/src/reference.ts Outdated Show resolved Hide resolved
hsubox76
hsubox76 approved these changes Aug 13, 2024
View reviewed changes
packages/storage/src/reference.ts Outdated Show resolved Hide resolved
egilmorez
egilmorez approved these changes Aug 14, 2024
View reviewed changes
Copy link
Contributor

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@dlarocque dlarocque merged commit 6b0ca77 into main Aug 14, 2024
33 of 35 checks passed
@dlarocque dlarocque deleted the dlarocque/storage-stream branch August 14, 2024 18:23
@google-oss-bot google-oss-bot mentioned this pull request Aug 14, 2024
Merged
markarndt
markarndt reviewed Aug 14, 2024
View reviewed changes
Copy link
Collaborator

@markarndt markarndt left a comment

Choose a reason for hiding this comment

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

LGTM.

@firebase firebase locked and limited conversation to collaborators Sep 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@markarndt markarndt markarndt left review comments

@hsubox76 hsubox76 hsubox76 approved these changes

@egilmorez egilmorez egilmorez approved these changes

@maneesht maneesht Awaiting requested review from maneesht maneesht is a code owner

@tonyjhuang tonyjhuang Awaiting requested review from tonyjhuang tonyjhuang is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

getStream() function has error: stream.pipe is not a function
5 participants
@dlarocque @google-oss-bot @egilmorez @hsubox76 @markarndt