-
Notifications
You must be signed in to change notification settings - Fork 85
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
refactor(eas-cli): swap node-fetch
for undici
to support Node 22 better
#2414
base: main
Are you sure you want to change the base?
Conversation
/changelog-entry bug-fix Swapped |
Size Change: +288 kB (+0.55%) Total Size: 52.2 MB
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2414 +/- ##
==========================================
+ Coverage 53.45% 53.58% +0.14%
==========================================
Files 530 530
Lines 19509 19513 +4
Branches 3968 3968
==========================================
+ Hits 10427 10455 +28
+ Misses 8330 8311 -19
+ Partials 752 747 -5 ☔ View full report in Codecov by Sentry. |
fetch, | ||
fetchOptions: (): RequestInit => { | ||
// @ts-expect-error - Type '(url: RequestInfo, init?: RequestInit) => Promise<Response>' is not assignable to type '{ (input: RequestInfo | URL, init?: RequestInit | undefined): Promise<Response>; (input: string | Request | URL, init?: RequestInit | undefined): Promise<...>; }'. | ||
fetch: (url: RequestInfo, init?: RequestInit) => |
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.
Does this fix the type error?
fetch: (url: RequestInfo, init?: RequestInit) => | |
fetch: (url: RequestInfo | URL, init?: RequestInit) => |
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.
Unfortunately not, it seems that typescript interprets the @types/node
differently.
TL;DR;
- in Node, they use
url: string | URL | globalThis.Request
(see here) - in Undici, they use
url: RequestInfo
, wheretype RequestInfo = string | URL | Request
(see here)
These should not conflict, but I have a feeling that it may be caused by this class extension in @types/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.
Since we might be dropping support for Node 16 (see PR #2413), we could just avoid importing fetch
from undici though. That should resolve this typing issue, and remove the need for the nock
<> undici
mock/workaround as well.
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.
Opening a stacked PR for this, we need to refactor more than just removing the import { fetch } from 'undici'
. This stacked PR may or may not succeed, depending on how much work still is required.
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.
Ok, so there is an issue. The summary is:
FormData
is used from theform-data
package, which is old and usesutil.isArray
(last update was 3y ago)form-data
is different fromundici
'sFormData
/ Node built-inFormData
form-data
supports Node'sfs.createReadStream
instances (ReadStream
types)undici
does NOT support Node'sReadStream
type, and only works withFile
(added in18.13.0
) or Blob (added in14.x.x
)- Starting from Node
20.x.x
, there is afs.openAsBlob
method to stream files as blobs, and pass it toFormData
, but we support Node 18 still
There are rather "dirty" hacks to work around this limitation, e.g. this one. I also have concerns with undici
's FormData
not sending the Content-Length
header.
Right now, undici
also doesn't play nice with the form-data
package. We'd need to add a Readable.toWeb(form)
to make it work.
For now, I've reached my timebox. Happy to circle back to this later though.
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.
Here is an example request made, when using the workaround from SO. It's still missing the Content-Length
header when streaming. When using a blob (in-memory read), it adds the header.
While, originally, this is the request made:
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.
Node 18's end of life will be in April 2025. So it's not soon, but eventually we'll be able to rely on fs.openAsBlob
. Could you document these findings in a task assigned to yourself so that it's easier to follow up on this after we've dropped Node 18?
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.
One workaround for Node 18 that I've found to work well is this one:
// Create a FormData object, and populate data
const form = new FormData();
const file = await createBlobFromPath(testFile);
form.append('some-value', 'value');
form.append('file', file, path.basename(testFile));
// Use `fs.openAsBlob` if available, else polyfill on Node 18
async function createBlobFromPath(filePath) {
if (typeof fs.openAsBlob === 'function') {
console.log('Using fs.openAsBlob:', filePath);
return await fs.openAsBlob(filePath);
}
console.log('Falling back to File polyfill:', filePath);
const { size } = await fs.promises.stat(filePath);
return {
[Symbol.toStringTag]: 'File', // Trick Node into thinking this is a File reference
get type() { return undefined }, // Always undefined, it's the mime type
get size() { return size }, // Ensure the `content-length` header is set through form data
stream() { return Readable.toWeb(fs.createReadStream(filePath)) }, // The magic part that makes it work
// Not necessary as we override the filename through formdata
// get name() { return name },
// Could add the full API, but it's not necessary for form data
// arrayBuffer(...args) { return this.stream().arrayBuffer(...args) },
// text(...args) { return this.stream().text(...args) },
// slice(...args) { return this.stream().slice(...args) }
};
}
This seems to run fine on Node 18 for ubuntu, macos, and windows. It's still leveraging Node 20+ APIs, but with a weird fallback. It's not spec-compliant, but enough compliant for FormData
itself, and includes the content-length
header we need.
598f48d
to
e5c4811
Compare
✅ Thank you for adding the changelog entry! |
This version includes the `File` primitive that might be required for FormData and large file uploads. See: https://nodejs.org/en/blog/release/v18.13.0\#introduce-file See: https://github.com/expo/eas-cli/pull/2414\#discussion_r1634640527
Why
Fixes ENG-12411
It also brings us closer to Node's own
fetch
implementation, based onundici
and is fetch-spec compliant. This is to improve Node 22 support, without warnings of deprecated system packages.How
Cherry-picked fix: update lockfile to only include, required for typechecks@types/node@20.11.0
#2412→ landed & rebased
Cherry-picked chore!: drop support for Node 16 #2413required for tests (Node 18+)→ landed & rebased
node-fetch
forundici
nock
to14.0.0-beta.7
for built-in fetch supportundici.fetch
issue,nock
is only attaching toglobal.fetch
(see here)Test Plan
See tests in GitHub Actions