Improve some types from Deno #1759
Conversation
see `TextDecoderStream`/`TextEncoderStream` types
see `ReadableStream` constructor type
|
Thanks for the PR! This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged. |
This reverts commit ee94bb6.
inputfiles/overridingTypes.jsonc
Outdated
| @@ -2573,7 +2573,7 @@ | |||
| }, | |||
| { | |||
| "name": "strategy", | |||
| "overrideType": "{ highWaterMark?: number }" | |||
| "overrideType": "{ highWaterMark?: number; size?: undefined }" | |||
There was a problem hiding this comment.
Why an unused property? Who would be happy with this? Can you add a comment about that? (it's jsonc so it supports comment)
There was a problem hiding this comment.
Hmm okay, but doesn't TypeScript already show error for excessive fields?
There was a problem hiding this comment.
It's a minor detail, but we can make new ReadableStream({ type: “bytes” }, { size: undefined }) be inferred as ReadableStream<Uint8Array>.
There was a problem hiding this comment.
Hmm that's interesting... Is that something frequently done by users?
There was a problem hiding this comment.
In general, I believe that implementations that set properties to undefined are often used because it's easy to write, while spread syntax is required to implement empty properties in some cases.
const option1 = {
foo: "foo",
bar: setBar ? "bar" : undefined,
};
const option2 = {
foo: "foo",
...(setBar ? { bar: "bar" } : {}),
};There was a problem hiding this comment.
But what is the hypothetical setBar here? readAsBytes? I would expect either always pass size or never pass size here than your conditional pass example, unless the caller also has the hypothetical setBar for type, which would be kinda complex setup to create ReadableStream. Especially when the way to read the resulting stream would be significantly different.
There was a problem hiding this comment.
I think it is nonsense to consider whether or not to give ?: undefined per API; it is consistent to allow for the undefined assignment, as in TypeScript's Partial. However, if you have strong concerns here, I am not opposed to revert.
There was a problem hiding this comment.
I think it is nonsense to consider whether or not to give
?: undefinedper API; it is consistent to allow for theundefinedassignment, as in TypeScript'sPartial. However, if you have strong concerns here, I am not opposed to revert.
Agreed that it's nonsense to decide per API (or at least hard to maintain), and I think it should not exist for consistency sake because it would be the first case if we add this here. That is unless we have a strong proof that there's many users who do setBar-style thing here (which I think doesn't make sense at least for this one).
There was a problem hiding this comment.
I think it should not exist for consistency sake because it would be the first case if we add this here. That is unless we have a strong proof that there's many users who do setBar-style thing here (which I think doesn't make sense at least for this one).
Agreed. I reverted this change.
There was a problem hiding this comment.
Thanks, I'll see whether #1713 is ready enough to be merged and when I can land this.
|
#1713 should probably just rebuild if merge conflict happens, so I'll just proceed here. LGTM, thanks! |
|
Merging because @saschanaz is a code-owner of all the changes - thanks! |
I am working on making the Deno types match the DOM types in this repository as closely as possible (denoland/deno#24599). I send a PR for the types that I think should be included in the TypeScript side.