Use null instead of Deno.EOF for read return types

[nayeemrmn]

Throughout cli/js and std, there are return types that look like number | Deno.EOF, Uint8Array | Deno.EOF, ServerRequest | Deno.EOF. These are a pain to use and force a proprietary type for no reason. With null we can use JS's null checking syntaxes to extract the important type.

[kevinkassimo]

Past discussions: denoland/std#444
(There was quite a bit discussion on null)

[kitsonk]

Also, I believe we could add assertNotEOF() in order to narrow types to remove it I believe.

[nayeemrmn]

I also want to point out that null just became a lot more powerful with nullish coalescing etc.

I think a custom null symbol is only needed for unions with generic types: T | NULL_SYMBOL where the lib needs to be able to tell apart NULL_SYMBOL from a possibly nullable user-supplied type. Deno.EOF just seems redundant, and Deno should avoid proprietary symbols where possible.

[kitsonk]

Ok, I have a slightly stronger opinion on this now... fetch() returns a Response::body and Body::read() returns a Promise<number | Deno.EOF>... This leaks the Deno namespace into non "standards" based code which is bad, because workers do not include the Deno namespace, but do include fetch() according to our typings, and that means they would have no way in theory of checking for Deno.EOF.

I have no problem with Deno.EOF for other Deno APIs, but it certainly should leak into other things.

cc/ @bartlomieju

[nayeemrmn]

Wait, why is there a Body::read()? That's not standard.

deno/cli/js/fetch.ts

Line 38 in 87c329c

class Body implements domTypes.Body, domTypes.ReadableStream, io.ReadCloser {

?

It might be useful to attach Deno specific functions to web APIs, we should use Deno.symbols for that.

[bartlomieju]

@kitsonk @nayeemrmn that's interesting topic, just yesterday I talked with @ry about it.

I'm not sure that fetch should return a body that is a resource in the first place.
This leads to the fact that you need to consume fetch's body or else it will leak (#3982).
Does it work like that in the browser?

EDIT: It looks like it does work like that; the only difference is that once Response is garbage collected underlying stream is discarded/closed. We'll need to figure this is out.

As for null instead of Deno.EOF I guess it makes sense given the example presented.

[nayeemrmn]

@ry Thoughts on this? Not much time left to make the change.

[ry]

@nayeemrmn I'm working on it.

[bartlomieju]

Discussed this issue yesterday - we have a sentiment to remove Deno.EOF however it might cause more harm than good. Namely: if we remove Deno.EOF in favor of null then there are 2 very distinct "falsy" values returned from Deno.read() - null and 0 - this might lead to unexpected behavior if conditional check is sloppy:

const nread = Deno.readSync(10, buf);

// bad:
if (!nread) { // both `null` and `0` will pass this conditional

}

// good:
if (typeof nread !== "number") {}

if (nread === null) {}

[kitsonk]

this might lead to unexpected behavior if conditional check is sloppy

As in the problem with all of JavaScript, right? The problem is that it makes it really hard to provide spec compliant features with the current behaviour.