Repository: denoland/deno (language: rust)

Issue:
require not defined error when cjs module in npm package has no file extension
Version: Deno 2.1.0

Last working version `2.0.6`.
```console
$ docker run --rm -it denoland/deno:alpine-2.0.6 run -A npm:@informalsystems/quint@0.22.4 --version
Warning The following packages contained npm lifecycle scripts (preinstall/install/postinstall) that were not executed:
┠─ npm:protobufjs@7.4.0
┃
┠─ This may cause the packages to not work correctly.
┠─ Lifecycle scripts are only supported when using a `node_modules` directory.
┠─ Enable it in your deno config file:
┖─ "nodeModulesDir": "auto
0.22.4
```

But for `>=2.1.0`, I am getting error for commonjs.
```console
$ docker run --rm -it denoland/deno:alpine-2.1.0 run -A npm:@informalsystems/quint@0.22.4 --version
Warning The following packages contained npm lifecycle scripts (preinstall/install/postinstall) that were not executed:
┠─ npm:protobufjs@7.4.0
┃
┠─ This may cause the packages to not work correctly.
┠─ Lifecycle scripts are only supported when using a `node_modules` directory.
┠─ Enable it in your deno config file:
┖─ "nodeModulesDir": "auto"
error: Uncaught (in promise) ReferenceError: require is not defined
    at file:///deno-dir/npm/registry.npmjs.org/yargs/17.7.2/yargs:3:69
    at loadESMFromCJS (node:module:761:21)
    at Module._compile (node:module:722:12)
    at Object.Module._extensions..js.Module._extensions..ts.Module._extensions..jsx.Module._extensions..tsx (node:module:754:10)
    at Module.load (node:module:662:32)
    at Function.Module._load (node:module:534:12)
    at Module.require (node:module:681:19)
    at require (node:module:797:16)
    at Object.<anonymous> (file:///deno-dir/npm/registry.npmjs.org/@informalsystems/quint/0.22.4/dist/src/cli.js:16:33)
    at Object.<anonymous> (file:///deno-dir/npm/registry.npmjs.org/@informalsystems/quint/0.22.4/dist/src/cli.js:356:4)

    info: Deno supports CommonJS modules in .cjs files, or when the closest
          package.json has a "type": "commonjs" option.
    hint: Rewrite this module to ESM,
          or change the file extension to .cjs,
          or add package.json next to the file with "type": "commonjs" option.
    docs: https://docs.deno.com/go/commonjs

$ docker run --rm -it denoland/deno:alpine-2.1.1 run -A npm:@informalsystems/quint@0.22.4 --version
< same error as before >
```

In `2.1.2`, I do get a suggestion for `--unstable-detect-cjs`.
```console
$ docker run --rm -it denoland/deno:alpine-2.1.2 run -A npm:@informalsystems/quint@0.22.4 --version
Warning The following packages contained npm lifecycle scripts (preinstall/install/postinstall) that were not executed:
┠─ npm:protobufjs@7.4.0
┃
┠─ This may cause the packages to not work correctly.
┠─ Lifecycle scripts are only supported when using a `node_modules` directory.
┠─ Enable it in your deno config file:
┖─ "nodeModulesDir": "auto"
error: Uncaught (in promise) ReferenceError: require is not defined
    at file:///deno-dir/npm/registry.npmjs.org/yargs/17.7.2/yargs:3:69
    at loadESMFromCJS (node:module:777:21)
    at Module._compile (node:module:722:12)
    at loadMaybeCjs (node:module:770:10)
    at Object.Module._extensions..js (node:module:761:12)
    at Module.load (node:module:662:32)
    at Function.Module._load (node:module:534:12)
    at Module.require (node:module:681:19)
    at require (node:module:812:16)
    at Object.<anonymous> (file:///deno-dir/npm/registry.npmjs.org/@informalsystems/quint/0.22.4/dist/src/cli.js:16:33)

    info: Deno supports CommonJS modules in .cjs files, or when the closest
          package.json has a "type": "commonjs" option.
    hint: Rewrite this module to ESM,
          or change the file extension to .cjs,
          or add package.json next to the file with "type": "commonjs" option,
          or pass --unstable-detect-cjs flag to detect CommonJS when loading.
    docs: https://docs.deno.com/go/commonjs
 ```
 
 But it still doesn't work with `--unstable-detect-cjs` flag.
 
 ```console
$ docker run --rm -it denoland/deno:alpine-2.1.2 run --unstable-detect-cjs -A npm:@informalsystems/quint@0.22.4 --version
...
< still same error >
```



Discussion / hints:
Looks like, the error is coming from `deno_core`.

https://github.com/denoland/deno_core/blob/cf34ee3076d714b659b5c4e08a42e24515739794/core/modules/map.rs#L1299

Maybe something inside `v8__Module__Evaluate`?
I managed to make a small reproducible:

```ts
// demo.cjs
const _yargs = require("yargs/yargs");
```

with `package.json`:

```json
{ "dependencies": { "yargs": "^17.7.2" } }
```

```console
$ deno@2.1.2 run -A --node-modules-dir=auto --unstable-detect-cjs demo.cjs
error: Uncaught (in promise) ReferenceError: require is not defined
    at file:///home/foo/node_modules/.deno/yargs@17.7.2/node_modules/yargs/yargs:3:69
    at loadESMFromCJS (node:module:777:21)
    at Module._compile (node:module:722:12)
    at loadMaybeCjs (node:module:770:10)
    at Object.Module._extensions..js (node:module:761:12)
    at Module.load (node:module:662:32)
    at Function.Module._load (node:module:534:12)
    at Module.require (node:module:681:19)
    at require (node:module:812:16)
    at Object.<anonymous> (file:///home/foo/demo.cjs:2:16)

    info: Deno supports CommonJS modules in .cjs files, or when the closest
          package.json has a "type": "commonjs" option.
    hint: Rewrite this module to ESM,
          or change the file extension to .cjs,
          or add package.json next to the file with "type": "commonjs" option,
          or pass --unstable-detect-cjs flag to detect CommonJS when loading.
    docs: https://docs.deno.com/go/commonjs
```

The example with `express` from https://docs.deno.com/go/commonjs works fine with this.
It seems this issue occurs because the file `yargs` in `yargs@17.7.2` has no file extension.
@dsherret I am happy to open an PR. but I need some direction where to look at to solve this.
@dsherret any guidance? 🥺
> any guidance? 🥺

Search the codebase for the functions mentioned in the stack trace:

```
at file:///home/foo/node_modules/.deno/yargs@17.7.2/node_modules/yargs/yargs:3:69
    at loadESMFromCJS (node:module:777:21)
    at Module._compile (node:module:722:12)
    at loadMaybeCjs (node:module:770:10) <-- specifically, this one
```

I believe it's considering this file as esm because the type in the package.json is "type": "module". Maybe files without extensions are treated as cjs by node. I'm not sure.
Thanks. 🙏 This was helpful. I managed to run the above code successfully if I change the follow line

https://github.com/denoland/deno/blob/1b7719c5d62a9303c8687f6b07c44f39a0349ae9/ext/node/polyfills/01_require.js#L432

to

```ts
  return ".cjs";
```

I'm not sure about making the change there because the the referrer impacts whether the module is considered cjs or esm and it would make any ESM modules without an extension not work with `require`.

I think probably we should probably make the change around here to prefer `ResolutionMode::Require` when `MediaType::from_path(&path) == MediaType::Unknown`: https://github.com/denoland/deno/blob/1b7719c5d62a9303c8687f6b07c44f39a0349ae9/resolvers/deno/cjs.rs#L280

That will cause it to take the slow path of attempting to load as CJS before loading as ESM if it's ESM.
I may have commented too early 😅 my previous message meant as a confirmation that I tracked the issue as per your suggestion.

I did trace it back exactly where you pointed. So, I guess, the refactor would be make `check_based_on_pkg_json` take `media_type` argument and do a condition here? But it is being used some other places, so I guess, I will make it optional.

This doesn't look like a pretty-fix. But if it's alright with you, I can open a PR.

PS. the fix does work !
Or, I can check if `path`

https://github.com/denoland/deno/blob/1b7719c5d62a9303c8687f6b07c44f39a0349ae9/resolvers/deno/cjs.rs#L270-L281

has no extension.

```rs
        Ok(if is_file_location_cjs || path.extension().is_none() {
          ResolutionMode::Require
        } else {
          ResolutionMode::Import
        })
```


Fix patch:
diff --git a/resolvers/deno/cjs.rs b/resolvers/deno/cjs.rs
index 76bc08e2bceb3b..f9f38e8b4381bf 100644
--- a/resolvers/deno/cjs.rs
+++ b/resolvers/deno/cjs.rs
@@ -274,7 +274,7 @@ impl<TInNpmPackageChecker: InNpmPackageChecker, TSys: FsRead>
         self.pkg_json_resolver.get_closest_package_json(&path)?
       {
         let is_file_location_cjs = pkg_json.typ != "module";
-        Ok(if is_file_location_cjs {
+        Ok(if is_file_location_cjs || path.extension().is_none() {
           ResolutionMode::Require
         } else {
           ResolutionMode::Import


Test patch:
diff --git a/tests/specs/node/import_cjs_file_no_ext_mjs_pkg/__test__.jsonc b/tests/specs/node/import_cjs_file_no_ext_mjs_pkg/__test__.jsonc
new file mode 100644
index 00000000000000..db0303092f79f8
--- /dev/null
+++ b/tests/specs/node/import_cjs_file_no_ext_mjs_pkg/__test__.jsonc
@@ -0,0 +1,4 @@
+{
+  "args": "run main.mts",
+  "output": "3\n"
+}
diff --git a/tests/specs/node/import_cjs_file_no_ext_mjs_pkg/main.mts b/tests/specs/node/import_cjs_file_no_ext_mjs_pkg/main.mts
new file mode 100644
index 00000000000000..7ae40ce0007e71
--- /dev/null
+++ b/tests/specs/node/import_cjs_file_no_ext_mjs_pkg/main.mts
@@ -0,0 +1,3 @@
+import { add } from "package/add";
+
+console.log(add(1, 2));
diff --git a/tests/specs/node/import_cjs_file_no_ext_mjs_pkg/node_modules/package/add b/tests/specs/node/import_cjs_file_no_ext_mjs_pkg/node_modules/package/add
new file mode 100644
index 00000000000000..674993901d8e84
--- /dev/null
+++ b/tests/specs/node/import_cjs_file_no_ext_mjs_pkg/node_modules/package/add
@@ -0,0 +1,1 @@
+module.exports.add = require("./internal.cjs").add;
diff --git a/tests/specs/node/import_cjs_file_no_ext_mjs_pkg/node_modules/package/internal.cjs b/tests/specs/node/import_cjs_file_no_ext_mjs_pkg/node_modules/package/internal.cjs
new file mode 100644
index 00000000000000..62c45aa26dc616
--- /dev/null
+++ b/tests/specs/node/import_cjs_file_no_ext_mjs_pkg/node_modules/package/internal.cjs
@@ -0,0 +1,1 @@
+module.exports.add = (a, b) => a + b;
diff --git a/tests/specs/node/import_cjs_file_no_ext_mjs_pkg/node_modules/package/package.json b/tests/specs/node/import_cjs_file_no_ext_mjs_pkg/node_modules/package/package.json
new file mode 100644
index 00000000000000..aead43de364cd1
--- /dev/null
+++ b/tests/specs/node/import_cjs_file_no_ext_mjs_pkg/node_modules/package/package.json
@@ -0,0 +1,3 @@
+{
+  "type": "module"
+}
\ No newline at end of file
diff --git a/tests/specs/node/import_cjs_file_no_ext_mjs_pkg/package.json b/tests/specs/node/import_cjs_file_no_ext_mjs_pkg/package.json
new file mode 100644
index 00000000000000..0967ef424bce67
--- /dev/null
+++ b/tests/specs/node/import_cjs_file_no_ext_mjs_pkg/package.json
@@ -0,0 +1,1 @@
+{}