Deno.emit panics with some types of custom scheme URLs

[kt3k]

I tested some patterns of custom scheme urls as input of Deno.emit, and I found the following 2 cause panic:

let id = 'custom://a.ts';
await Deno.emit(id, { sources: { [id]: "" } }); // panics with UnsupportedImportType

let id = 'custom://a//a.ts';
await Deno.emit(id, { sources: { [id]: "" } }); // => panics with AssertionError

(These types of urls are used in dext. ref: https://github.com/lucacasonato/dext.ts/blob/8483d19/src/plugins/dext.ts#L46 )

On the other hand, the following compiles without problem:

let id = 'custom://a/a.ts';
await Deno.emit(id, { sources: { [id]: "" } }); // => compiles

What do you think are the expected behaviors for the above 2 cases?

[kitsonk]

The first two I don't believe are valid URLs. Deno.emit now handles all specifiers via the Rust URL struct (Deno ModuleSpecifier) which gives things like relative resolution when resolving modules.

Any custom schemes that aren't specified require to be in the format of {scheme}://{hostname}?/{path} of which the first two do not meet that requirement.

We shouldn't panic though, we should reject the promise.

[nayeemrmn]

Does the second custom://a//a.ts not satisfy that format? It seems usable as a base. I wasn't sure about the first one since it technically has an empty pathname, but apparently it can also be handled by rust-url:

use url::Url;

let url = Url::parse("custom://a.ts").unwrap();
assert!(!url.cannot_be_a_base());
let url = Url::parse("custom://a//a.ts").unwrap();
assert!(!url.cannot_be_a_base());

Also:

• https://jsdom.github.io/whatwg-url/#url=Li9mb28=&base=Y3VzdG9tOi8vYS50cw==
• https://jsdom.github.io/whatwg-url/#url=Li9mb28=&base=Y3VzdG9tOi8vYS8vYS50cw==

[kitsonk]

If that is the case, @kt3k can you provide more details about what panics are happening and where?

[kt3k]

The first one comes from here https://github.com/denoland/deno/blob/81d73f2/cli/module_graph.rs#L1821, and the second one from here https://github.com/denoland/deno/blob/1a9209d/cli/tsc/99_main_compiler.js#L268

The first case looks just ok to throw JS error. Probably we shouldn't throw for the second case? I'll look into the second case more.

After reading @nayeemrmn's comment carefully, I feel both could be valid urls 🤔

[kitsonk]

Yes, I agree... what appears to be failing is the media type detection.

It is detected here:

https://github.com/denoland/deno/blob/81d73f2987c65463c0f2653f31d3d29d8db1986c/cli/specifier_handler.rs#L494

which comes from:

https://github.com/denoland/deno/blob/81d73f2987c65463c0f2653f31d3d29d8db1986c/cli/media_type.rs#L64-L78

So the proper fix would be to add examples of these valid, but incorrectly typed specifiers to the test cases and fix the media type detection. That should fix both panics I suspect.

[kt3k]

@kitsonk Sorry for changing the opinion again about this, but after checking the URL spec and the behaviors of whatwg-url (npm module) and rust-url (our depencency), I think the parsed result of custom://a.ts should be like scheme=custom, host=a.ts, pathname=(empty) (which means the current handling is correct). And because the pathname is empty, the MediaType has to be 'Unknown' for this url. So I propose that Deno.emit should throw with specifier=custom://a.ts because we don't know how to handle this extensionless url. I sent denoland/deno#9302 for this fix.

[kitsonk]

@kt3k I am not sure I agree. It is entirely up to us how we decide to assign a media type to a URL. Why wouldn't the logic of "if the pathname is empty, attempt to look at the hostname" be inappropriate? We could also limit it only to schemas that aren't "known" like "https", "http", "file", "data".

[kt3k]

@kitsonk

Why wouldn't the logic of "if the pathname is empty, attempt to look at the hostname" be inappropriate?

I'm now working towards this direction. I was able to change the media type of foo://mod.ts to TypeScript with this commit, but this seems to have hit another problem. With the above change, op_load is called with the specifier foo://mod.ts.ts/ for unknown reason, and the source code is not found for this op call. That causes an assertion error (in line:268 of 99_main_comiler.js). I'm still investigating the cause of this situation, but do you have any idea about this?

BTW although denoland/deno#9302 doesn't address this issue, I think it still addresses another problem (I updated the test case to illustrate what it fixes). So could you PTAL when you have time?

[kitsonk]

With the above change, op_load is called with the specifier foo://mod.ts.ts/ for unknown reason, and the source code is not found for this op call.

That is because we stringify the module specifier when we go into TypeScript, and it would appear that Url properly adds a trailing slash to a URL with only a hostname and no path. I am not sure of the best way to address it at the moment. 😢

[kt3k]

Memo:

I found that typescript normalizes the path during handling op_load call, ref: https://github.com/microsoft/TypeScript/blob/0383b5cb4c96083c12ea1453bf129fe675abaac9/src/compiler/program.ts#L2230-L2232
which replaces, for example, '//' to '/' in the middle of path:

const id = "https://example.com//a.ts";
await Deno.emit(id, { sources: { [id]: "" } });

In the above call emit requests op_load with specifier=https://example.com//a.ts but typescript changes it to https://example.com/a.ts in the middle of processing, and therefore source code is not found. This causes the panic of the 2nd case. I tried to skip path normalization in 00_typescript.js, but it didn't work.

Now this looks very difficult to resolve...

[kitsonk]

@kt3k I had some challenges with this before, in a different way. We might be able to work around it. We already have a construct for dealing with special handling of root modules because TypeScript refuses to accept some module specifiers as root modules.