Typing improvements for io and fixes for websocketclient, and a few others#1429
Typing improvements for io and fixes for websocketclient, and a few others#1429cmidgley wants to merge 25 commits intoModdable-OpenSource:publicfrom
io and fixes for websocketclient, and a few others#1429Conversation
…cket extend Device; improvements overall on types
…ts), fix bug with masks on fragmented reads, and add mask decode to control messages
cmidgley
changed the title
io and fixes for websocketclient`, and a few othersio and fixes for websocketclient, and a few others
There was a problem hiding this comment.
I'm not sure about this change. It must fix a problem you see. But, it also would require the caller to set host, port, and path in the options object, which isn't what we want.
There was a problem hiding this comment.
Good catch - on review I see I misunderstood what it was doing. I'll fix...
|
Fixes are in place. Let me know what else you find.. |
- tls extends tcp instead of full definition - Add 'undefined' as possible `read` result
| set format(format: string); | ||
| get format(): string; | ||
| } | ||
| export default class TLSSocket extends TCP {} |
There was a problem hiding this comment.
I'm no TypeScript expert, but don't you need to define that the TLSSocket constructor uses TLSOptions instead of TCPOptions for the options object argument?
There was a problem hiding this comment.
Thank you for filling this in. The change is good. I don't want even more copies of the HTTP status code map in the repo. I'll make a module for those so it can be shared. But that doesn't need to hold this up.
There was a problem hiding this comment.
419 UDP only specifies udp.read() - no argument and returns ArrayBuffer augmented with port and address. It does not specify multicast (though that's definitely on the roadmap)
| socket: TCPDevice | TLSDevice; | ||
| port?: number; | ||
| host: string; | ||
| dns: DNSUDPDevice; |
There was a problem hiding this comment.
There's no requirement that this is a DNS using UDP. It could be another resolution transport. But, for the moment this is probably OK?
There was a problem hiding this comment.
I'd say we add a generic DNS type when we have more, and change ClientOptions to use that (or just DNSUDPDevice | DNSOtherDevice here). Either way, it won't be a breaking change as it is just expanding the type. But using 'any' (or 'undefined') isn't a good alternative as it isn't strongly typed.
…pstream changes), fix TLSSocket constructor type, remove paramaterized UDP.read
…ce is defined; remove udp.read(buffer); add TLSSocket.constructor with TLSOptions definition
|
Merged latest code and resolved conflicts. Reviewed Ready for review and possible merging. |
|
Finally coming back around to this.... there's so much here that it is a little challenging to review. I'm not asking that you split it up, but it is a bit of an adventure to understand and review. That said, a lot of it looks good and we should get it merged. The |
2 participants
This is the work I've done to date with
iofocused on makingwebsocketclientto be reliable and strongly typed with TypeScript. It's a fairly substantial set of changes, mostly in the.d.tsfiles but some implementation (especiallywebsocketclient).Type changes:
embedded:io/socket/tcp/tls,embedded:network/http/server,embedded:network/http/server/options/webpage,embedded:network/https/server/options/websocket, andembedded:network/websocket/clientembedded:io/socket/listener,embedded:io/socket/tcp,embedded:io/socket/udp,embedded:network/dns/resolver/udp,embedded:network/http/client,embedded:provider/builtinDevice(for global variabledevice) into global types to eliminate requirement for importDevicefor when imports of extended modules are used (support forembedded:network/http/server,embedded:network/websocket/client)Implementation changes:
websocketclient:onError(error: string)(new feature)websocketwhere using attached sockets would fail (reported inembedded:io/socket/listenerlocks up Win simulator #1428)httpserveron theHTTP/1.1 <status> <code>headerembedded:network/http/server/options/webpageto support.contentTypeand.status.I typed everything based on the current implementation and not
ECMA-419(many modules are inconsistent with the published 2nd edition of the spec).