-
Notifications
You must be signed in to change notification settings - Fork 928
V11 release removal of node-fetch and undici dependencies #8492
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
Changes from 21 commits
4bdab6f
45723bb
5ee8eb7
84f0770
afea52d
79753b2
ba32125
6fdd758
0601594
2c4a46e
3b93ffe
97eb7a3
9c010b2
69d789c
c8c64e7
b16903b
0c05f70
5077ae3
7a6691b
033c4dc
9aa1f75
f35d2a0
1e25ff2
7acb14e
498bc7b
10a6149
10e089e
d517b14
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,15 +23,10 @@ | |
*/ | ||
export * from './index'; | ||
import { FetchProvider } from '@firebase/auth/internal'; | ||
import { | ||
fetch as undiciFetch, | ||
Headers as undiciHeaders, | ||
Response as undiciResponse | ||
} from 'undici'; | ||
import './index'; | ||
|
||
FetchProvider.initialize( | ||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Done! Thanks! |
||
Headers as unknown as typeof Headers, | ||
Response as unknown as typeof Response | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,17 +28,12 @@ import { AuthImpl } from '../core/auth/auth_impl'; | |
|
||
import { FetchProvider } from '../core/util/fetch_provider'; | ||
import { getDefaultEmulatorHost } from '@firebase/util'; | ||
import { | ||
fetch as undiciFetch, | ||
Headers as undiciHeaders, | ||
Response as undiciResponse | ||
} from 'undici'; | ||
|
||
// Initialize the fetch polyfill, the types are slightly off so just cast and hope for the best | ||
FetchProvider.initialize( | ||
undiciFetch as unknown as typeof fetch, | ||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||
Response as unknown as typeof Response | ||
); | ||
|
||
// First, we set up the various platform-specific features for Node (register | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,6 @@ | |
*/ | ||
|
||
import { Token } from '../../api/credentials'; | ||
import { DatabaseInfo } from '../../core/database_info'; | ||
import { Stream } from '../../remote/connection'; | ||
import { RestConnection } from '../../remote/rest_connection'; | ||
import { mapCodeFromHttpStatus } from '../../remote/rpc_error'; | ||
|
@@ -28,17 +27,6 @@ import { StringMap } from '../../util/types'; | |
* (e.g. `fetch` or a polyfill). | ||
*/ | ||
export class FetchConnection extends RestConnection { | ||
/** | ||
* @param databaseInfo - The connection info. | ||
* @param fetchImpl - `fetch` or a Polyfill that implements the fetch API. | ||
*/ | ||
constructor( | ||
databaseInfo: DatabaseInfo, | ||
private readonly fetchImpl: typeof fetch | ||
) { | ||
super(databaseInfo); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
|
||
openStream<Req, Resp>( | ||
rpcName: string, | ||
token: Token | null | ||
|
@@ -56,7 +44,7 @@ export class FetchConnection extends RestConnection { | |
let response: Response; | ||
|
||
try { | ||
response = await this.fetchImpl(url, { | ||
response = await fetch(url, { | ||
method: 'POST', | ||
headers, | ||
body: requestJson | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,9 +15,8 @@ | |
* limitations under the License. | ||
*/ | ||
import { registerFunctions } from './config'; | ||
import { fetch as undiciFetch } from 'undici'; | ||
|
||
export * from './api'; | ||
|
||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
DellaBitta marked this conversation as resolved.
Show resolved
Hide resolved
|
||
registerFunctions(undiciFetch as any, 'node'); | ||
registerFunctions('node'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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. |
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.