-
Notifications
You must be signed in to change notification settings - Fork 889
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
Conversation
🦋 Changeset detectedLatest commit: d517b14 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
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 |
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1Affected Products
Test Logs |
packages/auth-compat/index.node.ts
Outdated
fetch as undiciFetch, | ||
Headers as undiciHeaders, | ||
Response as undiciResponse | ||
} from 'undici'; | ||
import './index'; | ||
|
||
FetchProvider.initialize( |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this 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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
} | ||
resolve(); | ||
const reader = resp.body?.getReader(); | ||
reader?.read().then(function readChunk({ done, value }): any { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Auth changes LGTM
packages/auth-compat/index.node.ts
Outdated
undiciFetch as unknown as typeof fetch, | ||
undiciHeaders as unknown as typeof Headers, | ||
undiciResponse as unknown as typeof Response | ||
fetch as unknown as typeof fetch, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reviewed packages/firestore/*
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
variantsundici
andnode-fetch
for our node target builds and our CI tools.Testing
CI.
API Changes
NA.