Deno: TextDecoder.decode() uncaught error when passed null

[asos-craigmorten]

Noticed while upgrading a module to use [email protected] and Deno 1.3.1 (REF) that the TextDecoder decode() method is missing a null check resulting in the following error:

$ deno

Deno 1.3.1
exit using ctrl+d or close()
> let d = new TextDecoder()
undefined
> d.decode(null)
Uncaught TypeError: Cannot use 'in' operator to search for 'buffer' in null
    at TextDecoder.decode (/Users/runner/work/deno/deno/op_crates/web/08_text_encoding.js:445:18)
    at <unknown>:3:3
    at evaluate (rt/40_repl.js:60:36)
    at replLoop (rt/40_repl.js:160:15)

(response.arrayBuffer() from a fetch now returns null for a null response body in Deno 1.3.1, whereas it returned an empty string before in Deno 1.3.0, hence how saw this error)

Assuming we want to keep the fall-through behaviour currently implemented (anything invalid passed is overwritten with new Uint8Array(0)), but without the error thrown, just need to add a one-liner and test for passing null to the decode method.

REF: https://github.com/denoland/deno/blob/master/op_crates/web/08_text_encoding.js#L445

      let bytes;
      if (input instanceof Uint8Array) {
        bytes = input;
      } else if (isEitherArrayBuffer(input)) {
        bytes = new Uint8Array(input);
      } else if (
        typeof input === "object" &&
        input !== null && // <-- Add this here?
        "buffer" in input &&
        isEitherArrayBuffer(input.buffer)
      ) {
        bytes = new Uint8Array(
          input.buffer,
          input.byteOffset,
          input.byteLength,
        );
      } else {
        bytes = new Uint8Array(0);
      }

It's a small thing, and expect most folks will be protected via the type defs, but simple enough to trip into / reproduce with a Deno server of your choice, e.g. using an express like server:

import { opine } from "https://deno.land/x/[email protected]/mod.ts";

const app = opine();
app.get("/", (_req, res) => {
  res.setStatus(304).send();
});
const server = app.listen();
const address = server.listener.addr as Deno.NetAddr;
const url = `http://localhost:${address.port}`;

const decoder = new TextDecoder();
const res = await fetch(url);
// Response {
//   _bodySource: null,
//   _stream: null,
//   url: "http://localhost:51780",
//   statusText: "Not Modified",
//   status: 304,
//   headers: Headers { content-length: 0, x-powered-by: Opine },
//   redirected: false,
//   type: "default"
// }
const buf = await res.arrayBuffer(); // returns null
const decoded = decoder.decode(buf); // BOOM
// error: Uncaught TypeError: Cannot use 'in' operator to search for 'buffer' in null
//     at TextDecoder.decode (/Users/runner/work/deno/deno/op_crates/web/08_text_encoding.js:445:18)
//     at asd.ts:18:19
console.log(decoded);
server.close();

Happy to put in a small PR 🙃

[lucacasonato]

We should follow what browsers do here, which is throw a TypeError if the input is not a ArrayBuffer or ArrayBufferView. The error message could definitely be improved though. We should not default to Uint8Array(0) ever - that entire fallback should be removed, and replaced by a error. You can see this bad defaulting when passing a string.

Chrome:

const decoder = new TextDecoder()
decoder.decode(null)

Uncaught TypeError: Failed to execute 'decode' on 'TextDecoder': The provided value is not of type '(ArrayBuffer or ArrayBufferView)'

decoder.decode("")

Uncaught TypeError: Failed to execute 'decode' on 'TextDecoder': The provided value is not of type '(ArrayBuffer or ArrayBufferView)'

Firefox:

const decoder = new TextDecoder()
decoder.decode(null)

Uncaught TypeError: TextDecoder.decode: Argument 1 could not be converted to any of: ArrayBufferView, ArrayBuffer.

decoder.decode("")

Uncaught TypeError: TextDecoder.decode: Argument 1 could not be converted to any of: ArrayBufferView, ArrayBuffer.

Deno:
```
const decoder = new TextDecoder()
decoder.decode(null)

Uncaught TypeError: Cannot use 'in' operator to search for 'buffer' in null

decoder.decode("")

[asos-craigmorten]

@lucacasonato fair! It struck me as odd that didn't throw as per spec (REF: https://encoding.spec.whatwg.org/#dom-textdecoder-decode) / current browser implementations.

~Will close my PR as it follows the incorrect precedent opposed to spec.~

Update: will re-open and update actually, thinking on it... throwing a TypeError in the else statement won't be much work! (assuming that is all that is needed).

[asos-craigmorten]

In addition to above and https://github.com/denoland/deno/issues/7181 should also consider the undefined case - in both Chrome and Firefox this is supported with a return value of an empty string "".