Conversation
| remote: URL, | ||
| method: string, | ||
| body: BodyInit | null, | ||
| headers: BareHeaders, |
There was a problem hiding this comment.
the js headers class forbids certain headers and doesn't let you have 2 values per key
There was a problem hiding this comment.
Is there any case when the order of the headers (and the case of the header name) actually matters? In that case, you'd want [string, string][] and not a Map<string, string | string[]>.
| body: BodyInit | null, | ||
| headers: BareHeaders, | ||
| signal: AbortSignal | undefined | ||
| ) => Promise<SmallResponse>; |
There was a problem hiding this comment.
Response has a bunch of methods, but then again, you can construct Responses with ease...
There was a problem hiding this comment.
better to let switcher construct the response, less code duplication. plus we want to return BareHeaders not headers so
There was a problem hiding this comment.
Ah, good point. Also, generally, the switcher shouldn't construct the response either, as the proxy might have its own implementation of Response.
| onopen: () => void, | ||
| onmessage: (data: ArrayBuffer | string) => void, | ||
| onclose: (code: number, reason: string) => void, | ||
| onerror: (error: string) => void, |
There was a problem hiding this comment.
If you mean Node event emitters, then that's a huge dependency for each Bare client, and no one is crazy enough to use EventTarget.
There was a problem hiding this comment.
that makes the types much more confusing, and there would only be one listener
| export interface BareTransport { | ||
| connect: ( | ||
| url: URL, | ||
| protocols: [string], |
There was a problem hiding this comment.
This is not how you specify a string array
There was a problem hiding this comment.
well it is, but it's not what i wanted here
There was a problem hiding this comment.
Yeah, you wanted string[], and I missed that, whoops.
|
|
||
| export type SmallResponse = | ||
| { | ||
| body: ReadableStream | ArrayBuffer | Blob | string, |
There was a problem hiding this comment.
instanceof? i don't see the need to specify
There was a problem hiding this comment.
that's bad practice in this case imo
almost always you should be returning a ReadableStream even if you output a single buffer to it and end it there
There was a problem hiding this comment.
I was about to disagree with @Semisol, but he's actually fully in the right. I was about to point out that ReadableStreams are annoying to construct, but every response should be treated as a stream, as the response body is usually (read: almost always) sent over multiple messages/datagrams/whatever.
| { | ||
| body: ReadableStream | ArrayBuffer | Blob | string, | ||
| headers: BareHeaders, | ||
| status: number |
There was a problem hiding this comment.
up to the bare switcher to add
There was a problem hiding this comment.
I'm fairly certain statusText is from the server's response, so it should be added.
|
|
||
| export type BareHeaders = Map<string, string | string[]>; | ||
|
|
||
| export type SmallResponse = |
There was a problem hiding this comment.
Is this all that's needed for responses?
| url: URL, | ||
| protocols: [string], | ||
| onopen: () => void, | ||
| onmessage: (data: ArrayBuffer | string) => void, |
There was a problem hiding this comment.
| onmessage: (data: ArrayBuffer | string) => void, | |
| onmessage: (data: Blob | string) => void, |
Blobs can be made into ArrayBuffers (and vice versa) with relative ease, but they're also better for large messages. Plus, WSes themselves have an option to use Blobs instead of ArrayBuffers, so this benefit isn't something I'm making up.
There was a problem hiding this comment.
Actually, I've changed my mind. ArrayBuffers might make more sense, as constructing a Blob requires all the parts to be present in memory anyway.
| body: ReadableStream | ArrayBuffer | Blob | string, | ||
| headers: BareHeaders, | ||
| status: number | ||
| } |
There was a problem hiding this comment.
Personally, I would yoink the prototype interface from lib.dom.d.ts, remove the readonly keywords, and make some properties optional
| onerror: (error: string) => void, | ||
| ) => (data) => void; | ||
|
|
||
| // somethingforwebtransportsidk: (???)=>??? |
|
|
||
| // somethingforwebtransportsidk: (???)=>??? | ||
|
|
||
| request: ( |
There was a problem hiding this comment.
I would prefer this method being named "fetch"
|
|
||
| export type BareHeaders = Map<string, string | string[]>; | ||
|
|
||
| export type SmallResponse = |
There was a problem hiding this comment.
I would prefer this type being called "MinimalResponse"
4 participants
No description provided.