TextEncoder.encode TypeError: Invalid argument should give more details

[Noah-Huppert]

I believe that the TypeError returned by TextEncoder.encode is too ambiguous. I think for any other method which isn't dealing with text encoding an ambiguous error message like Invalid argument would be okay. However text encoding is full of many hairy input situations, and more details to narrow down which bad input situation you are experiencing would be very helpful.

---

Here is a short story highlighting where just a little more detail in the error message could have helped a lot:

I had an error where some code before a call to encode() was accidentally returning objects instead of strings. It was hard for me to find because I even had Typescript annotations on the argument I was passing to encode() asserting that the argument should be a string, like so:

let foo = shouldReturnAStringButReturnedAnObject();

function whichCallsEncodeWithArg(arg: string) {
  // arg never touched in any way until here
  new TextEncoder.encode(arg);
}

whichCallsEncodeWithArg(foo);

However type checks were passed and the error occurred.

This small self contained example effectively reproduces passing an object to encode():

new TextEncoder().encode({});

The resulting error is something like:

error: Uncaught TypeError: Invalid argument
    at TextEncoder.encode (deno:op_crates/web/08_text_encoding.js:1066:21)
    at main.ts:171:27
    at Array.forEach (<anonymous>)
    at main.ts:170:8

Due to the ambiguaty of the error, and my confidence is what turned out to be false assumptions about the type of an argument, I went down a long rabbit hole trying to figure out if my program was producing any invalid unicode glyphs or something like that. I think it would be helpful for encode() to throw an error more like TypeError: Invalid argument, must be a string.

[kitsonk]

Actually we should be more spec compliant here...

new TextEncoder().encode({})

Results in the following in Chrome:

Uint8Array(15) [91, 111, 98, 106, 101, 99, 116, 32, 79, 98, 106, 101, 99, 116, 93]

It is only values that are not coercible to strings that throw. For example:

new TextEncoder().encode(Symbol("a"));

Results in:

Uncaught TypeError: Cannot convert a Symbol value to a string

We should leave the type enforcement up to TypeScript.

[Noah-Huppert]

Thanks for the quick reply. Yea that seems reasonable. Definitely agree that the type checking should be left up to typescript. Just to be clear the details about types and such was more of an illustration of a tricky debugging situation where a more verbose error would have been nice, and not meant to put any emphasis on type checking.

Since you labeled this as good first issue I thought I'd point readers to the TextEncodersource code: https://github.com/denoland/deno/blob/master/op_crates/web/08_text_encoding.js#L1061 (Although I suspect the meat of the issue is actually the Deno.core.encode() implementation that TextEncoder uses in most cases).

[kitsonk]

Well, I don't think we shouldn't worry about Deno.core.encode(). It should be a slightly better error on line 604:

deno/core/bindings.rs

Lines 596 to 609 in 2a36e2f

	fn encode(
	scope: &mut v8::HandleScope,
	args: v8::FunctionCallbackArguments,
	mut rv: v8::ReturnValue,
	) {
	let text = match v8::Local::<v8::String>::try_from(args.get(0)) {
	Ok(s) => s,
	Err(_) => {
	let msg = v8::String::new(scope, "Invalid argument").unwrap();
	let exception = v8::Exception::type_error(scope, msg);
	scope.throw_exception(exception);
	return;
	}
	};

But that isn't as important as it is that we should try to coerce to a string in TextEncoder.encode():

deno/op_crates/web/08_text_encoding.js

Lines 1063 to 1082 in 2a36e2f

	encode(input = "") {
	// Deno.core.encode() provides very efficient utf-8 encoding
	if (this.encoding === "utf-8") {
	return core.encode(input);
	}
	const encoder = new UTF8Encoder();
	const inputStream = new Stream(stringToCodePoints(input));
	const output = [];
	while (true) {
	const result = encoder.handler(inputStream.read());
	if (result === "finished") {
	break;
	}
	output.push(...result);
	}
	return new Uint8Array(output);
	}

Like something on 1064 that just does input = String(input). That should likely generate a proper TypeError anyways.

[benjamingr]

I'll take a look at this - better TypeError doesn't sound correct but @kitsonk 's fix actually seems to generate the correct behaviour (of coercing to string). In the web:

encoder.encode({toString() { return "text" }}); // returns Uint8Array(4) [116, 101, 120, 116]

I think Deno should behave like the WHATWG spec requires - but still use string in its own d.ts for better dx.