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

V11 release removal of node-fetch and undici dependencies #8492

Open
wants to merge 25 commits into
base: v11
Choose a base branch
from

Conversation

DellaBitta
Copy link
Contributor

@DellaBitta DellaBitta commented Sep 10, 2024

Discussion

Our v11 release will require node 18+. Since fetch has been introduced in these node versions, we can remove our dependency on third party fetch implementations.

This PR removes our usage of fetch variants undici and node-fetch for our node target builds and our CI tools.

Testing

CI.

API Changes

NA.

Copy link

changeset-bot bot commented Sep 10, 2024

⚠️ No Changeset found

Latest commit: 498bc7b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Sep 10, 2024

Size Report 1

Affected Products

  • @firebase/auth

    TypeBase (a377fb2)Merge (7f0c024)Diff
    main179 kB178 kB-74 B (-0.0%)
  • @firebase/auth-compat

    TypeBase (a377fb2)Merge (7f0c024)Diff
    main29.5 kB29.4 kB-74 B (-0.3%)
  • @firebase/auth/internal

    TypeBase (a377fb2)Merge (7f0c024)Diff
    main214 kB214 kB-74 B (-0.0%)
  • @firebase/firestore-lite

    TypeBase (a377fb2)Merge (7f0c024)Diff
    browser111 kB111 kB-56 B (-0.1%)
    esm5108 kB108 kB-25 B (-0.0%)
    main153 kB153 kB-156 B (-0.1%)
    module111 kB111 kB-56 B (-0.1%)
    react-native111 kB111 kB-56 B (-0.1%)
  • @firebase/functions

    TypeBase (a377fb2)Merge (7f0c024)Diff
    browser9.76 kB9.65 kB-113 B (-1.2%)
    esm512.1 kB12.0 kB-113 B (-0.9%)
    main12.8 kB12.6 kB-163 B (-1.3%)
    module9.76 kB9.65 kB-113 B (-1.2%)
  • @firebase/rules-unit-testing

    TypeBase (a377fb2)Merge (7f0c024)Diff
    main17.8 kB17.6 kB-228 B (-1.3%)
  • @firebase/storage

    TypeBase (a377fb2)Merge (7f0c024)Diff
    main59.4 kB59.3 kB-90 B (-0.2%)
  • bundle

    TypeBase (a377fb2)Merge (7f0c024)Diff
    firestore-lite (Query Cursors)91.3 kB91.2 kB-56 B (-0.1%)
    firestore-lite (Query)87.4 kB87.3 kB-56 B (-0.1%)
    firestore-lite (Read data once)62.9 kB62.8 kB-53 B (-0.1%)
    firestore-lite (Transaction)88.1 kB88.1 kB-56 B (-0.1%)
    firestore-lite (Write data)72.5 kB72.4 kB-56 B (-0.1%)
    functions (call)32.0 kB31.9 kB-49 B (-0.2%)
  • firebase

    TypeBase (a377fb2)Merge (7f0c024)Diff
    firebase-compat.js791 kB791 kB-55 B (-0.0%)
    firebase-firestore-lite.js119 kB119 kB-56 B (-0.0%)
    firebase-functions-compat.js8.22 kB8.17 kB-52 B (-0.6%)
    firebase-functions.js12.0 kB11.9 kB-49 B (-0.4%)

Test Logs

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

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Sep 10, 2024

Size Analysis Report 1

Affected Products

  • @firebase/functions

    • connectFunctionsEmulator

      Size

      TypeBase (a377fb2)Merge (7f0c024)Diff
      size2.50 kB2.46 kB-39 B (-1.6%)
      size-with-ext-deps20.3 kB20.3 kB-39 B (-0.2%)
    • getFunctions

      Size

      TypeBase (a377fb2)Merge (7f0c024)Diff
      size2.68 kB2.64 kB-39 B (-1.5%)
      size-with-ext-deps28.1 kB28.1 kB-39 B (-0.1%)
    • httpsCallable

      Size

      TypeBase (a377fb2)Merge (7f0c024)Diff
      size6.13 kB6.08 kB-49 B (-0.8%)
      size-with-ext-deps24.0 kB23.9 kB-49 B (-0.2%)
    • httpsCallableFromURL

      Size

      TypeBase (a377fb2)Merge (7f0c024)Diff
      size6.08 kB6.03 kB-49 B (-0.8%)
      size-with-ext-deps23.9 kB23.9 kB-49 B (-0.2%)

Test Logs

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

@DellaBitta DellaBitta changed the title WIP V11 release removal of node-fetch and undici dependencies V11 release removal of node-fetch and undici dependencies Sep 13, 2024
@DellaBitta DellaBitta marked this pull request as ready for review September 13, 2024 20:55
fetch as undiciFetch,
Headers as undiciHeaders,
Response as undiciResponse
} from 'undici';
import './index';

FetchProvider.initialize(
Copy link
Contributor Author

@DellaBitta DellaBitta Sep 16, 2024

Choose a reason for hiding this comment

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

Keeping the FetchProvider because it's used in by auth's testing framework.

databaseInfo: DatabaseInfo,
private readonly fetchImpl: typeof fetch
) {
super(databaseInfo);
Copy link
Contributor Author

@DellaBitta DellaBitta Sep 16, 2024

Choose a reason for hiding this comment

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

This consructor is removed because it would now have the same signature as RestConnection's constructor.

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.

Thanks, looks very thorough! Just a few questions.

undiciHeaders as unknown as typeof Headers,
undiciResponse as unknown as typeof Response
fetch as unknown as typeof fetch,
Headers as unknown as typeof Headers,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still have to do the casting or can it infer that it's a typeof, well, what it is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


export * from './api';

// eslint-disable-next-line @typescript-eslint/no-explicit-any
registerFunctions(undiciFetch as any, 'node');
registerFunctions('node');
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I don't think we actually need a separate functions Node bundle anymore. Maybe that can be a separate PR though. The only thing different is that it labels the bundle "node" for platform logging, which is pointless if it's the same bundle. I guess it tells us how many node users are using functions. Also it doesn't export the types from public-types which is probably a mistake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add the export.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We spoke offline about removing the separate node bundle, will be done in a different PR.

packages/rules-unit-testing/test/util.test.ts Show resolved Hide resolved
packages/storage/test/node/connection.test.ts Show resolved Hide resolved
}
resolve();
const reader = resp.body?.getReader();
reader?.read().then(function readChunk({ done, value }): any {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a chance of this hanging if there's no reader or no body? Do we have to call a resolve() or reject() or downloadFailed() or something at the end as a failsafe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thank you! Updated the code.

Copy link
Contributor

@NhienLam NhienLam left a comment

Choose a reason for hiding this comment

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

Auth changes LGTM

undiciFetch as unknown as typeof fetch,
undiciHeaders as unknown as typeof Headers,
undiciResponse as unknown as typeof Response
fetch as unknown as typeof fetch,
Copy link
Contributor

Choose a reason for hiding this comment

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

As Christina pointed out in the other comment, we also don't need casting here, correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants