-
Notifications
You must be signed in to change notification settings - Fork 3
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
Fix issue with node envs (must explicitly set duplex
option on Request constructror)
#152
Fix issue with node envs (must explicitly set duplex
option on Request constructror)
#152
Conversation
commit:
|
duplex
option on Request constructror0duplex
option on Request constructror)
tested by Nele on her/oscar's bun project, where the issue first came up |
7c9d9e1
to
0696491
Compare
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.
Can confirm it works on the bun project 👍
…e Deno does not like our stackblitz packages
0696491
to
d2fa57b
Compare
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.
Oh it's been merged 😅 I didn't finish my review fast enough . I noticed one small thing though
@@ -660,3 +660,13 @@ function FailedWebsocket() { | |||
</div> | |||
); | |||
} | |||
|
|||
const safeParseJson = (jsonString: string) => { |
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.
There's already an eerily similar exact same safeParseJson
function in src/utils/index.ts
. Why not import & use that?
Alright. So. In environments that use node/undici, we need to conform to specifications APPARENTLY.
The fetch specification is in a weird spot.
Technically, when the body of a
RequestInit
is a ReadableStream, you MUST specify the propertyduplex: "half"
There will be future support for
duplex: "full"
, however this is not yet exposed.So really, the only option for
duplex
is"half"
but we need to use it explicitly for future compatibility.