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

Replace node-fetch dependency with undici #7705

Merged
merged 44 commits into from
Nov 12, 2023
Merged

Replace node-fetch dependency with undici #7705

merged 44 commits into from
Nov 12, 2023

Conversation

DellaBitta
Copy link
Contributor

@DellaBitta DellaBitta commented Oct 17, 2023 •
edited
Loading

Discussion

Update our dependency on aging node-fetch v2.6.7 to undici 5.26.5.

This should fix some security vulnerabilities within node-fetch as well as fix user issue #7660.

Testing

  • CI Tests
  • Local tests, adding console.log output to ensure the undici paths are either executed and/or don't have any resolution conflicts in non-node based apps. Scenarios tested:
    • Node.js test app.
    • JavaScript test app (browser only).
    • Next.JS app, tested with SSR and CSR.
    • React app.

API Changes

N/A

@changeset-bot
Copy link

changeset-bot bot commented Oct 17, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: fcb842c

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

This PR includes changesets to release 9 packages
Name Type
@firebase/auth-compat Minor
@firebase/firestore Minor
@firebase/functions Minor
@firebase/storage Minor
@firebase/auth Minor
firebase Minor
@firebase/storage-compat Patch
@firebase/firestore-compat Patch
@firebase/functions-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 Oct 18, 2023 •
edited
Loading

Size Report 1

Affected Products

  • @firebase/app

    TypeBase (ff1a6ec)Merge (8696194)Diff
    browser14.4 kB14.5 kB+108 B (+0.8%)
    esm518.9 kB19.0 kB+108 B (+0.6%)
    main19.8 kB19.9 kB+108 B (+0.5%)
    module14.4 kB14.5 kB+108 B (+0.8%)

  • @firebase/auth

    TypeBase (ff1a6ec)Merge (8696194)Diff
    browser176 kB177 kB+559 B (+0.3%)
    cordova204 kB205 kB+875 B (+0.4%)
    esm5229 kB230 kB+875 B (+0.4%)
    main174 kB174 kB+604 B (+0.3%)
    module176 kB177 kB+559 B (+0.3%)
    react-native193 kB194 kB+1.03 kB (+0.5%)

  • @firebase/auth-compat

    TypeBase (ff1a6ec)Merge (8696194)Diff
    main29.6 kB29.5 kB-105 B (-0.4%)

  • @firebase/auth/cordova

    TypeBase (ff1a6ec)Merge (8696194)Diff
    browser204 kB205 kB+875 B (+0.4%)
    module204 kB205 kB+875 B (+0.4%)

  • @firebase/auth/internal

    TypeBase (ff1a6ec)Merge (8696194)Diff
    browser187 kB187 kB+559 B (+0.3%)
    esm5243 kB244 kB+875 B (+0.4%)
    main210 kB210 kB+618 B (+0.3%)
    module187 kB187 kB+559 B (+0.3%)

  • @firebase/firestore-lite

    TypeBase (ff1a6ec)Merge (8696194)Diff
    main150 kB150 kB-173 B (-0.1%)

  • @firebase/functions

    TypeBase (ff1a6ec)Merge (8696194)Diff
    main12.9 kB12.7 kB-173 B (-1.3%)

  • @firebase/storage

    TypeBase (ff1a6ec)Merge (8696194)Diff
    browser57.8 kB57.8 kB-18 B (-0.0%)
    esm564.4 kB64.3 kB-18 B (-0.0%)
    main59.6 kB59.4 kB-189 B (-0.3%)
    module57.8 kB57.8 kB-18 B (-0.0%)

  • bundle

    43 size changes

    TypeBase (ff1a6ec)Merge (8696194)Diff
    analytics (logEvent)43.8 kB43.9 kB+100 B (+0.2%)
    app-check (CustomProvider)36.6 kB36.7 kB+100 B (+0.3%)
    app-check (ReCaptchaEnterpriseProvider)39.1 kB39.2 kB+100 B (+0.3%)
    app-check (ReCaptchaV3Provider)39.1 kB39.2 kB+100 B (+0.3%)
    auth (Anonymous)73.1 kB73.5 kB+413 B (+0.6%)
    auth (EmailAndPassword)80.9 kB81.4 kB+413 B (+0.5%)
    auth (GoogleFBTwitterGitHubPopup)99.7 kB100 kB+413 B (+0.4%)
    auth (GooglePopup)97.0 kB97.4 kB+413 B (+0.4%)
    auth (GoogleRedirect)97.2 kB97.6 kB+413 B (+0.4%)
    auth (Phone)83.3 kB83.8 kB+413 B (+0.5%)
    database (Append to a list of data)148 kB148 kB+100 B (+0.1%)
    database (Filtering data)147 kB147 kB+100 B (+0.1%)
    database (Listen for child events)164 kB164 kB+100 B (+0.1%)
    database (Listen for value events + Detach listeners)164 kB164 kB+100 B (+0.1%)
    database (Listen for value events)163 kB164 kB+100 B (+0.1%)
    database (Read data once)163 kB163 kB+100 B (+0.1%)
    database (Save data as transactions)166 kB166 kB+100 B (+0.1%)
    database (Sort data)149 kB149 kB+100 B (+0.1%)
    database (Write data)147 kB148 kB+100 B (+0.1%)
    firestore (Persistence)302 kB302 kB+100 B (+0.0%)
    firestore (Query Cursors)238 kB238 kB+100 B (+0.0%)
    firestore (Query)236 kB236 kB+100 B (+0.0%)
    firestore (Read data once)224 kB224 kB+100 B (+0.0%)
    firestore (Realtime updates)226 kB226 kB+100 B (+0.0%)
    firestore (Transaction)204 kB204 kB+100 B (+0.0%)
    firestore (Write data)204 kB204 kB+100 B (+0.0%)
    firestore-lite (Query Cursors)88.8 kB88.9 kB+100 B (+0.1%)
    firestore-lite (Query)85.0 kB85.1 kB+100 B (+0.1%)
    firestore-lite (Read data once)61.3 kB61.4 kB+100 B (+0.2%)
    firestore-lite (Transaction)86.2 kB86.3 kB+100 B (+0.1%)
    firestore-lite (Write data)70.9 kB71.0 kB+100 B (+0.1%)
    functions (call)31.2 kB31.3 kB+100 B (+0.3%)
    messaging (send + receive)46.5 kB46.6 kB+100 B (+0.2%)
    performance (trace)51.0 kB51.1 kB+100 B (+0.2%)
    remote-config (getAndFetch)45.5 kB45.6 kB+100 B (+0.2%)
    storage (getBytes)41.3 kB41.4 kB+100 B (+0.2%)
    storage (getDownloadURL)43.4 kB43.5 kB+100 B (+0.2%)
    storage (getMetadata)42.8 kB42.9 kB+100 B (+0.2%)
    storage (list + listAll)42.2 kB42.3 kB+100 B (+0.2%)
    storage (updateMetadata)43.1 kB43.2 kB+100 B (+0.2%)
    storage (uploadBytes)48.2 kB48.0 kB-153 B (-0.3%)
    storage (uploadBytesResumable)58.1 kB58.0 kB-153 B (-0.3%)
    storage (uploadString)48.4 kB48.2 kB-153 B (-0.3%)

  • firebase

    TypeBase (ff1a6ec)Merge (8696194)Diff
    firebase-app-compat.js28.9 kB29.0 kB+94 B (+0.3%)
    firebase-app.js93.4 kB93.5 kB+152 B (+0.2%)
    firebase-auth-compat.js136 kB136 kB+277 B (+0.2%)
    firebase-auth-cordova.js173 kB174 kB+745 B (+0.4%)
    firebase-auth.js146 kB147 kB+432 B (+0.3%)
    firebase-compat.js778 kB778 kB+365 B (+0.0%)
    firebase-performance-standalone-compat.es2017.js90.1 kB90.2 kB+100 B (+0.1%)
    firebase-performance-standalone-compat.js67.3 kB67.4 kB+100 B (+0.1%)
    firebase-storage-compat.js41.3 kB40.3 kB-986 B (-2.4%)
    firebase-storage.js46.4 kB46.2 kB-246 B (-0.5%)

Test Logs

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

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Oct 18, 2023 •
edited
Loading

Size Analysis Report 1

This report is too large (359,154 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/Q0VivttV5N.html

@DellaBitta DellaBitta changed the title Update node-fetch dependency to 3.2.0 Replace node-fetch dependency with Undici Oct 29, 2023
@DellaBitta DellaBitta changed the title Replace node-fetch dependency with Undici Replace node-fetch dependency with undici Oct 29, 2023
hsubox76
hsubox76 reviewed Nov 3, 2023
View reviewed changes
.changeset/smart-maps-wash.md Outdated
'@firebase/firestore': patch
'@firebase/functions': patch
'@firebase/storage': patch
'@firebase/auth': patch
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be invisible from the user's perspective but it is a big change under the hood, so I am leaning towards minor. If users notice any bugs or incompatibilities with the upgrade (we have tested this pretty thoroughly so it would be in an environment or use case we haven't thought of) it's feels like it makes more sense to point to this happening with a minor bump.

packages/auth-compat/karma.conf.js Show resolved Hide resolved
packages/firestore/src/platform/node_lite/connection.ts Outdated Show resolved Hide resolved
packages/functions/src/index.node.ts Outdated Show resolved Hide resolved
packages/storage/src/platform/node/connection.ts Outdated Show resolved Hide resolved
repo-scripts/changelog-generator/package.json Outdated Show resolved Hide resolved
@firebase firebase deleted a comment from github-actions bot Nov 6, 2023
hsubox76
hsubox76 approved these changes Nov 7, 2023
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.

I think the outstanding failed test is just flaky - looks like timeouts.

MarkDuckworth
MarkDuckworth approved these changes Nov 7, 2023
View reviewed changes
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.

Firestore changes LGTM.

@DellaBitta DellaBitta requested a review from zwu52 as a code owner November 8, 2023 19:06
DellaBitta and others added 4 commits November 8, 2023 14:31
@github-actions GitHub Actions
Copy link
Contributor

Changeset File Check ⚠️

  • Warning: This PR modifies files in the following packages but they have not been included in the changeset file:%0A - @firebase/rules-unit-testing%0A%0A Make sure this was intentional.

@DellaBitta DellaBitta merged commit bebecda into master Nov 12, 2023
41 checks passed
@DellaBitta DellaBitta deleted the ddb-upgrade-node-fetch branch November 12, 2023 15:07
@google-oss-bot google-oss-bot mentioned this pull request Nov 12, 2023
Merged
@DellaBitta DellaBitta mentioned this pull request Nov 17, 2023
Closed
@firebase firebase locked and limited conversation to collaborators Dec 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@prameshj prameshj prameshj approved these changes

@hsubox76 hsubox76 hsubox76 approved these changes

@MarkDuckworth MarkDuckworth MarkDuckworth approved these changes

@avolkovi avolkovi Awaiting requested review from avolkovi

@sam-gc sam-gc Awaiting requested review from sam-gc

@yuchenshi yuchenshi Awaiting requested review from yuchenshi

@lisajian lisajian Awaiting requested review from lisajian

@renkelvin renkelvin Awaiting requested review from renkelvin

@maneesht maneesht Awaiting requested review from maneesht

@tonyjhuang tonyjhuang Awaiting requested review from tonyjhuang

@zwu52 zwu52 Awaiting requested review from zwu52

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

Successfully merging this pull request may close these issues.
5 participants
@DellaBitta @google-oss-bot @MarkDuckworth @hsubox76 @prameshj