Consider hard error when importing cjs

[dsherret]

Right now Deno will allow importing cjs/cts files... and then treat them as esm for some reason. I believe it should error instead because cjs is a node concept. The following currently works:

// main.js
import "./mod.cjs";
// mod.cjs
import * as path from "node:path";
console.log(path.SEP);

If we ever add support for cjs files then we should treat them like cjs and not esm.

We should also remove cjs/cts from deno test/bench autodiscovery.

[bartlomieju]

That seams reasonable. Preferably we'd do it as soon as possible.

[mash-graz]

Because I'm right now trying to rewrite the file/module format detection and preparation for V8 execution, and therefore had to study this question of CJS support as well, here my opinion about this issue:

sure -- we shouldn't invite users to just keep their old node user habits and never change to ESM, but we should also support compatibility if it doesn't waste to many efforts.

CJS import in ESM contexts looks like a clearly solvable task. The other way around it is much harder, because of the async requirement.

CJS import can be seen in principle as just wrapping pure JS code within a simple envelope and very view helper functions (see: https://nodejs.org/api/modules.html#the-module-wrapper). that's perfectly possible in deno as well. It was even rather user-friendly documented in those days, when node compatibility in deno required a lot more knowledge and workarounds (https://deno.land/[email protected]/node/README.md?source#commonjs-modules-loading).

Well -- let's forget theory and ideology concerning this topic for now.

I stumbled over this issue just by chance, because a small logical programming bug happing in https://github.com/denoland/deno/pull/19480/files#r1526571781 let my application taste the CJS import attempts of denos node:worker_threads accidentally.

And that's also the reason, why I think, CJS support in deno should not be strictly denied, but remain and very carefully provided for those rare cases, where it is simply necessary to support general node compatibility. node:worker_threads script import is indeed one of this practical cases.

Although the mentioned bug could be solved by simply adding the missing brackets to fix the intended logical operation, I have begun to replace the surrounding code by a more proper solution, which indeed should also support scripts, which expecting CJS import.

I try to reuse the url_to_node_resolution suggested by @dsherret to avoid code duplication of file/module format detection at different places and I would like to handle the whole section in rust and its URL and fs routines, to get rid of this very ugly local toFileUrl implementation used in worker_threads.ts right now.

Does this look appreciable to you?

[dsherret]

I try to reuse the url_to_node_resolution suggested by @dsherret to avoid code duplication of file/module format detection at different places and I would like to handle the whole section in rust and its URL and fs routines, to get rid of this very ugly local toFileUrl implementation used in worker_threads.ts right now.

For that issue I think we should just align/fix the resolution behaviour to be the same everywhere by reusing url_to_node_resolution.

[mash-graz]

yes -- I'm definitely utilizing this facility as one important part of the code! -- Although, a explicit .cjs-suffix-check, which has priority over a surrounding ESM context, is still missing there, too.

But right now I'm more puzzled about the question of input validation for workers filename specifieres. In the strict sense of the node documentation we should only expect filesystem paths in the string variant of this argument, but in practice -- just look around -- there are very often stringified URLs used there. And in older deno documentations it was even explicitly stated, that its implementation in this particular detail differs from node, by supporting html: and blob: URLs as well...