Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"module is not defined" cjs suggestion should not occur for mjs/mts modules #26557

Open
dsherret opened this issue Oct 25, 2024 · 5 comments
Open
Labels
bug Something isn't working correctly good first issue Good for newcomers

Comments

@dsherret
Copy link
Member

> cat main.mjs
import * as a from "./a.cjs";

console.log(a.add(1, 2));

module.isPreloading = true;
> deno run main.mjs
3                                                                                                                                                                                                                                                                                                                
error: Uncaught (in promise) ReferenceError: module is not defined                                                                                                                                                                                                                                                             
module.isPreloading = true;                                                                                                                                                                                                                                                                                                    
^
    at file:///V:/scratch/main.mjs:5:1

    info: Deno supports CommonJS modules in .cjs files, or when there's a package.json
          with "type": "commonjs" option and --unstable-detect-cjs flag is used.
    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
          and pass --unstable-detect-cjs flag.
    docs: https://docs.deno.com/go/commonjs

This suggestion doesn't make sense.

Node for example:

> node main.mjs
3
file:///V:/scratch/main.mjs:5
module.isPreloading = true;
^

ReferenceError: module is not defined in ES module scope
    at file:///V:/scratch/main.mjs:5:1
    at ModuleJob.run (node:internal/modules/esm/module_job:262:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:474:24)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:109:5)

Node.js v22.3.0
@dsherret dsherret added the bug Something isn't working correctly label Oct 25, 2024
@dsherret
Copy link
Member Author

@bartlomieju is this a good first issue?

Relevant code is here:

deno/runtime/fmt_errors.rs

Lines 307 to 309 in d92d2fe

if msg.contains("module is not defined")
|| msg.contains("exports is not defined")
|| msg.contains("require is not defined")

@SaiThanushreddy
Copy link

@dsherret Can I work on this??

@dsherret
Copy link
Member Author

Yup 👍

@SaiThanushreddy
Copy link

@dsherret To resolve the issue with suggesting CommonJS fixes for .mjs and .mts files, we can modify the get_suggestions_for_terminal_errors function to check the file extension. If the extension is .cjs, we show the CommonJS suggestions; if it’s .mjs or .mts, we skip these suggestions entirely. This will provide more accurate guidance based on the module type.

@bartlomieju
Copy link
Member

Yes, it's a good first issue.

@SaiThanushreddy looks like you got a proper solution. PRs are welcome :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants