-
Notifications
You must be signed in to change notification settings - Fork 0
Transport types #1
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
base: master
Are you sure you want to change the base?
Changes from all commits
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 | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,36 @@ | ||||||
export type GenericMeta = | ||||||
{ | ||||||
// ??? | ||||||
}; | ||||||
|
||||||
export type BareHeaders = Map<string, string | string[]>; | ||||||
|
||||||
export type SmallResponse = | ||||||
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 would prefer this type being called "MinimalResponse" |
||||||
{ | ||||||
body: ReadableStream | ArrayBuffer | Blob | string, | ||||||
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. How do you specify what type to get? 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. instanceof? i don't see the need to specify 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. that's bad practice in this case imo 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 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. |
||||||
headers: BareHeaders, | ||||||
status: number | ||||||
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. where is statusText? 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. up to the bare switcher to add 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'm fairly certain |
||||||
} | ||||||
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. Personally, I would yoink the prototype interface from lib.dom.d.ts, remove the readonly keywords, and make some properties optional |
||||||
|
||||||
export interface BareTransport { | ||||||
connect: ( | ||||||
url: URL, | ||||||
protocols: [string], | ||||||
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 is not how you specify a string array 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. See the 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. 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. that is valid syntax??? 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. well it is, but it's not what i wanted here 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. Yeah, you wanted |
||||||
onopen: () => void, | ||||||
onmessage: (data: ArrayBuffer | string) => void, | ||||||
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.
Suggested change
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
onclose: (code: number, reason: string) => void, | ||||||
onerror: (error: string) => void, | ||||||
Comment on lines
+19
to
+22
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. Just use an event emitter... 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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that makes the types much more confusing, and there would only be one listener 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. Use an EventTarget. |
||||||
) => (data) => void; | ||||||
|
||||||
// somethingforwebtransportsidk: (???)=>??? | ||||||
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 recommend using TODO's for this |
||||||
|
||||||
request: ( | ||||||
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 would prefer this method being named "fetch" |
||||||
remote: URL, | ||||||
method: string, | ||||||
body: BodyInit | null, | ||||||
headers: BareHeaders, | ||||||
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. Why not 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. the js headers class forbids certain headers and doesn't let you have 2 values per key 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. reasonable 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. 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 |
||||||
signal: AbortSignal | undefined | ||||||
) => Promise<SmallResponse>; | ||||||
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. Why not 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.
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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, good point. Also, generally, the switcher shouldn't construct the response either, as the proxy might have its own implementation of |
||||||
|
||||||
meta: () => GenericMeta | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
{ | ||
"name": "ThinClient", | ||
"version": "1.0.0", | ||
"description": "", | ||
"scripts": { | ||
"test": "echo \"Error: no test specified\" && exit 1" | ||
}, | ||
"keywords": [], | ||
"author": "", | ||
"license": "ISC" | ||
} |
Uh oh!
There was an error while loading. Please reload this page.
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.
Is this all that's needed for responses?