module: clarify esm error message in require calls

[dario-piotrowicz]

The current error message that is thrown when an ES module calls require simply suggests to "use import instead" this is slightly ambiguous since in JavaScript there are two types of imports: static and dynamic. This change simply clarifies the error message by clearly suggesting to use either a static or dynamic import

If someone is interested in a before-after screenshot:

Fixes #40544

Note

This PR replaces the stalled #53152, that PR provided more help to the user than mine does, I however softly disagree with that approach as I think we can assume a certain level of understanding by developers of how modules work, and if they don't it should be simple enough for them to refer to the docs.
So I am simply clarifying that both static and dynamic imports can be used here (I assume everyone knows what those are, if not, googling/mdning it should be pretty trivial) rather than throwing errors with long elaborate explanatory messages
If someone disagree and thinks that we should indeed provide more details to users I'm happy to try to get closer to the solution in #53152

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.23%. Comparing base (fb26fa1) to head (5fc38be).
Report is 15 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57774      +/-   ##
==========================================
- Coverage   90.24%   90.23%   -0.02%     
==========================================
  Files         630      630              
  Lines      185188   185245      +57     
  Branches    36275    36303      +28     
==========================================
+ Hits       167130   167157      +27     
- Misses      11001    11016      +15     
- Partials     7057     7072      +15     

Files with missing lines	Coverage Δ
lib/internal/modules/esm/module_job.js	98.27% <100.00%> (ø)

... and 30 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/66106/

[jsumners-nr]

As someone who rarely interacts with ESM, the new message is as clear as the old message.

[dario-piotrowicz]

As someone who rarely interacts with ESM, the new message is as clear as the old message.

Hi @jsumners-nr 🙂 , thanks for the comment, I understand that I am not adding much to the error message but as I mentioned in the PR description I was trying not to be too verbose and assume a minimal level of familiarity with the modules system and/or the possibility for developers to google the missing information.

Would you have a potential alternative error message in mind that you can suggest that can be clearer without it being too verbose? 🙂

[jsumners-nr]

Would you have a potential alternative error message in mind that you can suggest that can be clearer without it being too verbose? 🙂

Maybe, if I knew what either message was trying to tell me to do. Possibly include a link to documentation that explain the error and shows examples of correcting it?

[dario-piotrowicz]

Would you have a potential alternative error message in mind that you can suggest that can be clearer without it being too verbose? 🙂

Maybe, if I knew what either message was trying to tell me to do.

(very briefly and simplified) in nodejs there are two types of modules cjs and esm, in the former you can use require to import other cjs modules while in the latter you need to use imports, and there are two type of imports you can use static imports and dynamic imports. The current error message just says "use import instead" which I think might be a bit ambiguous (which imports? static ones? or dynamic ones? or both?), so I hope that me updating it to say "use a static or dynamic import instead" can help a bit.

Possibly include a link to documentation that explain the error and shows examples of correcting it?

That could be a valid idea but by looking at the code this seems like a pattern very seldom used in nodejs (I can only see a handful of such occurrences) 🤔 , I also think that a pattern like that has its own maintainability drawbacks since docs can change over time and any reference to them (AFAICT) needs potential manual efforts to keep in sync 🤔

[aduh95]

@dario-piotrowicz explaining it in a GH comment is fine, but I think the point of @jsumners-nr was that the error message should be more informative. I agree with that, if someone doesn't know what use import instead means, they're even less likely to understand what use a static or dynamic import instead means. Are there people out there who would understand the latter and not the former? I'd argue that the new message is even more confusing as static imports might not be possible if they're not at the top-level – and I'm not saying we should recommend dynamic imports, I'm saying the "vagueness" is a feature, not a bug

[jsumners-nr]

I agree with that, if someone doesn't know what use import instead means, they're even less likely to understand what use a static or dynamic import instead means.

Exactly. As I stated, I deal with ESM as little as possible. Differentiating between "static" and "dynamic" import statements, when I was clearly trying to use the simpler require method, is assuming too much knowledge. Presumably, the error wouldn't occur if the developer had the knowledge to fix the error because they wouldn't have written the code in a way to encounter the error.

Additionally, it's probably far more common for this error to be encountered because require ESM is no longer behind a flag. So what was once working is now throwing an obscure error that the developer might not understand.

In my opinion, if this is going to be improved, the most viable way I can see is by adding a permanent link to docs that outline the problem and show solutions. As for docs moving, there's a standard out there called "HTTP". In it, there are well defined error codes and mechanisms for content that has changed addresses. These are called redirects, and if any linked docs move in the future then they should be accompanied by an appropriate redirect. For example, https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/301.

[ljharb]

import has been standard syntax for a decade, and dynamic import since 2017 - no matter how little you deal with ESM or write it, I think it's a reasonable assumption that a node user will know what both kinds of import are, or at least know how to figure it out.

[jsumners-nr]

import has been standard syntax for a decade, and dynamic import since 2017 - no matter how little you deal with ESM or write it, I think it's a reasonable assumption that a node user will know what both kinds of import are, or at least know how to figure it out.

Why would such an assumption be reasonable? I am quite serious that I don't know what the proposed change is meant to make me understand. For me, a "dynamic import" looks something like:

const foundName = lookUpSomeName()
const importedModule = require(`${foundName}`)

Is that what the error message wants me to do?

The ESM import syntax is complicated, and has many permutations. I highly doubt I'm the only one who has learned the basic import something from 'a-thing' and then has to look up why that doesn't work for some things.

My point is: if the intention is to make the error message CLEAR, then the proposed change is insufficient.

[ljharb]

@jsumners-nr since you know that import is syntax, you at least know what an ESM import is, and if you see "dynamic or static" and are confused, i would hope you know how to look up on MDN or google, or ask an LLM, and you'd presumably learn the difference?

Things don't have to be clear immediately, as long as they can unambiguously lead to clarity.

I've never used or heard the term "dynamic import" to mean the same as "dynamic require", so that one really confuses me.

[jsumners-nr]

Things don't have to be clear immediately, as long as they can unambiguously lead to clarity.

I disagree.

I've never used or heard the term "dynamic import" to mean the same as "dynamic require", so that one really confuses me.

"import" means to include source code from somewhere else. It does not necessarily mean the import keyword.

[dario-piotrowicz]

Are there people out there who would understand the latter and not the former? I'd argue that the new message is even more confusing as static imports might not be possible if they're not at the top-level

Well I decided to go with the change exactly because the issue description in #40544 shows, I think, that the current error message confused the reporter which assumed that the only appropriate solution was a dynamic import. So updating it to clearly say to use one or the other should at least hopefully address that sort of confusion I believe/hope 🙂

I'm saying the "vagueness" is a feature, not a bug

I see... so I take it you'd prefer to keep the current wording? 🥲

(PS: from my point of view, I am not removing this "vagueness" feature, but just slightly reducing it to hopefully nudge people a bit more in the right direction)

[aduh95]

I'm saying the "vagueness" is a feature, not a bug

I see... so I take it you'd prefer to keep the current wording? 🥲

Yes I don't think the wording in this PR is decreasing potential confusion, so my preference would be to not land as is (but I don't feel strongly about it).
More importantly, I disagree that this PR is fixing the linked issue. To Jordan's point, the number of folks writing code in an ESM file who don't know what import is is ever decreasing – but to James' point, both error messages (current one and in this PR) leave to the user to figure out what to do. Linking to some document that explains the subtle differences between CJS and ESM would maybe address it, but we probably don't have the resources to maintain such docs.

[lpinca]

I think the improvement is the removal of "you can", making the tone more formal, but yes the message is not clearer.

[dario-piotrowicz]

I'm saying the "vagueness" is a feature, not a bug

I see... so I take it you'd prefer to keep the current wording? 🥲

Yes I don't think the wording in this PR is decreasing potential confusion, so my preference would be to not land as is (but I don't feel strongly about it).

I also don't feel too strongly about it landing, although I do think it marginally improves things but it clearly it's a super impactful change 🙂

More importantly, I disagree that this PR is fixing the linked issue.

Mh... I'm not completely sure I agree with that, as I mentioned in the issue I believe the reporter was asking to clarify that the tool to use was a dynamic import and the changes here are clarifying that you can use either a static or dynamic one, so that makes me think that this addressed to some extent the reporter's confusion 🤔

To Jordan's point, the number of folks writing code in an ESM file who don't know what import is is ever decreasing – but to James' point, both error messages (current one and in this PR) leave to the user to figure out what to do.

Agreed 👍

Linking to some document that explains the subtle differences between CJS and ESM would maybe address it, but we probably don't have the resources to maintain such docs.

Would you like me to go with that solution then? and/or what would your preference be here?

By the way, have you seen the PR I linked in my PR description: #53152
What do you think so something like that? (personally I find long verbose error messages a bit overwhelming/annoying but I'm curious to hear your thoughts on it)