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

Very confusing message when user doesn't realize they're in a CommonJS context #50009

Closed
DanielRosenwasser opened this issue Jul 22, 2022 · 16 comments · Fixed by #50088
Closed
Assignees
Labels
Domain: Error Messages The issue relates to error messaging Domain: ES Modules The issue relates to import/export style module behavior Effort: Casual Good issue if you're already used to contributing to the codebase. Harder than "good first issue". Fix Available A PR has been opened for this issue Help Wanted You can do this

Comments

@DanielRosenwasser
Copy link
Member

Found from an issue I was diagnosing for @mpodwysocki.

Today, if you use "module": "nodenext" or "module": "node16", you might try to import from a package with ESM files and get the following error message

Module 'yadda/file.js' cannot be imported using this construct. The specifier only resolves to an ES module, which cannot be imported synchronously. Use dynamic import instead.

This is extremely confusing. The fix is typically to add the following field to the package.json that corresponds to the importing file with the error:

    "type": "module"
@fatcerberus
Copy link

This becomes especially confusing because it’s an ESM import you’re writing (so the error is actively wrong to say it “cannot be imported using this construct”—it can, with the right package configuration). The error only makes sense when you know the import is being transpiled to require(), and even that’s confusing because it’s not actually a problem with the source code as written but instead the JS output.

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Jul 23, 2022

How about this.


If the file is .cts or .cjs extension:

The current file is a CommonJS module whose imports will produce require() calls; however, the referenced file is an ECMAScript module and cannot be imported via require(). Consider writing a dynamic import() call instead.

Related diagnostics:

The specifier {0} resolved to this file with the path {1}.

If the file does not have an .mts or .mjs extension and there is no type field set in package.json

The current file is a CommonJS module and cannot use this syntax to reference ECMAScript modules. It is likely that your 'package.json' needs a 'type' field set to 'module'.

Related diagnostics:

Consider adding `"type": "module"` to this 'package.json' file.
The specifier '{0}' resolved to this file with the path '{1}'.

If the file does not have an .mts or .mjs extension and there is type field set in package.json

According to this file's 'package.json', it is a CommonJS module and cannot use this syntax to reference ECMAScript modules. This file can be switched to an ECMAScript module by setting its extension to '{0}'.

Related diagnostics:

This file's 'package.json' lives here.
The specifier '{0}' resolved to this file with the path '{1}'.

@DanielRosenwasser DanielRosenwasser added Domain: Error Messages The issue relates to error messaging Domain: ES Modules The issue relates to import/export style module behavior Help Wanted You can do this Effort: Casual Good issue if you're already used to contributing to the codebase. Harder than "good first issue". labels Jul 25, 2022
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 4.8.1 milestone Jul 25, 2022
@DanielRosenwasser
Copy link
Member Author

It's possible we could just have the related info be "resolved to this file".

@andrewbranch
Copy link
Member

The specifier '{0}' resolved to this file with the path '{1}'.

Is this useful? In an editor scenario you can already go-to-definition on it, but I don’t see what you’d need to do in the resolved file as part of addressing the error.

@andrewbranch
Copy link
Member

I don’t think it’s appropriate to rule out dynamic import in any of these scenarios. I’m planning to use a single head message for all of them:

The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import(\"{0}\")' call instead.

and optionally attaching either

To convert this file to an ECMAScript module, change its file extension to '{0}'.

or

To convert this file to an ECMAScript module, change its file extension to '{0}', or add `\"type\": \"module\"` to its package.json file with path '{1}'.

(We cannot place a related diagnostic on the package.json file itself because it’s not a source file.)

@DanielRosenwasser
Copy link
Member Author

Sad - okay, what about

- or add `\"type\": \"module\"` to its package.json file with path '{1}'.
+ or add the field `\"type\": \"module\"` to '{1}'.

@andrewbranch
Copy link
Member

Actually, there is a createDetachedDiagnostic which looks like it might work. But is using that and splitting the one related info into two better than having them combined?

@DanielRosenwasser
Copy link
Member Author

I don’t think it’s appropriate to rule out dynamic import in any of these scenarios.

I don't know if there's ever been a context where I could truly use a dynamic import in the context where I needed it. That's why I'm doubtful that it's helpful in the first place as a suggestion.

That said, the related diagnostics can be smooshed.

@andrewbranch
Copy link
Member

That may be true, but converting everything to ESM is a more consequential change and may be even more of a hassle. I also considered moving the dynamic import suggestion to a related diagnostic so it feels more like it has equal weight with the other suggestions:

  • The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'.
    • To import "{0}" into this CommonJS module, use a dynamic 'import("{0}")' call.
    • To convert this file into an ECMAScript module, change its file extension to '{0}'.

@fatcerberus
Copy link

Consider writing a dynamic 'import("{0}")' call instead.

Note that this isn't a valid fix if the tsconfig specifies "module": "node" - then the dynamic import gets converted to require() too.

@luongjames8
Copy link

I'm on Typescript 4.9.3 and this doesn't seem to be fixed yet.

It is VERY confusing. I am glad that I found this thread, and hope the head message can be updated soon.

@alexherrera42
Copy link

It would be great if there was an option I could specify to silence this error. I'm importing ESM modules that I want typescript to recognize and process, but I'm not building with typescript. I'm using webpack, which handles all the import statements, which makes this error irrelevant noise.

@andrewbranch
Copy link
Member

--moduleResolution bundler

@alexherrera42
Copy link

alexherrera42 commented Feb 23, 2023

You're a life saver. Thank you. I did google, but I guess I figured it would've been something in the tsconfig. I appreciate your help.

@Jood80
Copy link

Jood80 commented May 20, 2023

 "compilerOptions": {
    "moduleResolution": "node",
    }

Should do the trick too.

@alexherrera42
Copy link

That will not do the trick when importing ESM.

@microsoft microsoft locked as resolved and limited conversation to collaborators May 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Domain: Error Messages The issue relates to error messaging Domain: ES Modules The issue relates to import/export style module behavior Effort: Casual Good issue if you're already used to contributing to the codebase. Harder than "good first issue". Fix Available A PR has been opened for this issue Help Wanted You can do this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants