search

fails-components / webtransport

Http/3 webtransport support for node
Other
149 stars 21 forks source link

fix: constrain quicheLogVerbose value #264

Closed achingbrain closed 7 months ago

achingbrain commented 7 months ago

The valid values are -1 | 0 | 1 | 2 | 3 so cause a type error if anything else is passed.

martenrichter commented 7 months ago

I am not sure where the actual limit is.... it is also just an upper bound. Here are the details for the possible values: https://github.com/google/quiche/blob/main/quiche/common/platform/default/quiche_platform_impl/quiche_logging_impl.h

achingbrain commented 7 months ago

It might be these ones? https://fuchsia.googlesource.com/third_party/abseil-cpp/+/refs/tags/20230125.3/absl/base/log_severity.h#69

martenrichter commented 7 months ago

Yes, this must be the one, and -1 is off. It is the default.

achingbrain commented 7 months ago

The types here are hard to use because the WebTransportBase constructor takes a WebTransportOptions rather than a HttpWebTransportInit so the quicheLogVerbose isn't a valid option.

It needs to be added to the WebTransportOptions interface which I see already has some non-standard options defined in it.

FWIW most of dom.js can be removed now since the WebTransport types are defined in TypeScript's DOM typings which they weren't back when dom.js was added.

martenrichter commented 7 months ago

The types here are hard to use because the WebTransportBase constructor takes a WebTransportOptions rather than a HttpWebTransportInit so the quicheLogVerbose isn't a valid option.

So far, I did not consider it a public interface..., but feel free to add it in a pact.

It needs to be added to the WebTransportOptions interface which I see already has some non-standard options defined in it.

FWIW most of dom.js can be removed now since the WebTransport types are defined in TypeScript's DOM typings which they weren't back when dom.js was added.

I do not think so. The WebTransport spec changes very often, so I would have missing properties. So it is better to keep it, as it will for sure miss some of the new options.

achingbrain commented 7 months ago

In that case it might be better to extend the built-in types to add new properties rather than redefining them?

Otherwise you can end up in all sorts of trouble when some field three levels down in an interface has a type that's missing a null or some other trivial thing.

martenrichter commented 7 months ago

Otherwise you can end up in all sorts of trouble when some field three levels down in an interface has a type that's missing a null or some other trivial thing.

Precisely, this happens already with the Streams, where the definition from node.js is slightly different from the dom one. And as this is the only project, I come in touch with types, it is taking too much time.

achingbrain commented 7 months ago

The Node.js types are quite tedious, yes. They are written by the community though, sometimes you have to PR DefinitelyTyped/DefinitelyTyped/types/node, but the maintainers there are at least pretty good about merging PRs quickly.