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

feat: replace node-fetch with @remix-run/web-fetch #2736

Merged
merged 54 commits into from
May 14, 2022
Merged

Conversation

jacob-ebey
Copy link
Member

@jacob-ebey jacob-ebey commented Apr 11, 2022

This PR replaces the internal usage of node-fetch with @remix-run/web-fetch to be more spec compliant and allow for ReadableStream's to be used as a body in node.

This work is critical to enable a runtime agnostic way to implement streaming in the server-runtime package and provide React 18 support to everyone.

I have also re-worked the file upload handler API to operate on an AsyncIterable for the body content. This allows for better error propagation as well as bringing unstable_parseMultipartFormData to all supported platforms, not just Node.

  • Docs
  • Tests

@jacob-ebey jacob-ebey marked this pull request as draft April 12, 2022 01:04
@jacob-ebey jacob-ebey changed the title feat: replace node-fetch with @web-std/fetch feat: replace node-fetch with @remix-run/web-fetch Apr 28, 2022
@jacob-ebey jacob-ebey marked this pull request as ready for review April 28, 2022 23:56
Copy link
Member

@mjackson mjackson left a comment

Choose a reason for hiding this comment

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

Great start. Keep going! Let's also look into getting rid of busboy and using our own multipart parser in web-std/form-data.

packages/remix-architect/server.ts Show resolved Hide resolved
packages/remix-express/package.json Outdated Show resolved Hide resolved
packages/remix-express/server.ts Show resolved Hide resolved
packages/remix-express/server.ts Outdated Show resolved Hide resolved
packages/remix-node/fetch.ts Outdated Show resolved Hide resolved
packages/remix-node/fetch.ts Show resolved Hide resolved
packages/remix-node/index.ts Outdated Show resolved Hide resolved
jacob-ebey added 3 commits May 4, 2022 11:12
feat: updated upload handlers to use new API
fix: clean up adapters
feat: remove abort controller from adapters and request
@jacob-ebey jacob-ebey requested a review from mjackson May 5, 2022 01:17
packages/remix-node/stream.ts Outdated Show resolved Hide resolved
packages/remix-node/stream.ts Outdated Show resolved Hide resolved
packages/remix-node/stream.ts Outdated Show resolved Hide resolved
packages/remix-node/stream.ts Outdated Show resolved Hide resolved
packages/remix-node/tsconfig.json Outdated Show resolved Hide resolved
@jacob-ebey jacob-ebey requested a review from mjackson May 13, 2022 01:20
docs: updated docs to include note about not reading body in loader
Copy link
Member

@mjackson mjackson left a comment

Choose a reason for hiding this comment

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

LGTM!

@jacob-ebey jacob-ebey dismissed MichaelDeBoey’s stale review May 13, 2022 23:53

Breaking unstable_ API should be fine.

@jacob-ebey jacob-ebey merged commit 25b4c83 into dev May 14, 2022
@jacob-ebey jacob-ebey deleted the jacob/web-std-fetch branch May 14, 2022 00:28
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-a114c40-20220514 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v1.5.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@dahlbyk
Copy link

dahlbyk commented Jun 29, 2022

Any obvious reason I'd be getting this after upgrading from 1.4? If not, I'll try to submit a repro or fix.

TypeError: this._readableStream.getReader is not a function
    at ReadableStreamSearch.[Symbol.asyncIterator] ({redacted}/node_modules/@web3-storage/multipart-parser/cjs/src/search.js:149:41)
    at AsyncGenerator.next (<anonymous>)
    at Object.streamMultipart ({redacted}/node_modules/@web3-storage/multipart-parser/cjs/src/index.js:119:29)
    at streamMultipart.next (<anonymous>)
    at parseMultipartFormData ({redacted}/node_modules/@remix-run/server-runtime/formData.js:48:18)

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

Successfully merging this pull request may close these issues.

4 participants