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

Better error message for using outdated moduleResolution: node(10) #55104

Closed
5 tasks done
JoshuaKGoldberg opened this issue Jul 21, 2023 · 10 comments Β· Fixed by #56949
Closed
5 tasks done

Better error message for using outdated moduleResolution: node(10) #55104

JoshuaKGoldberg opened this issue Jul 21, 2023 · 10 comments Β· Fixed by #56949
Assignees
Labels
Experience Enhancement Noncontroversial enhancements Fix Available A PR has been opened for this issue Suggestion An idea for TypeScript

Comments

@JoshuaKGoldberg
Copy link
Contributor

Suggestion

πŸ” Search Terms

moduleResolution node node10 eol esm commonjs cannot find types

βœ… Viability Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

When a TSConfig is using "moduleResolution": "node" or "node10" and an import resolves to an npm package whose types declarations that doesn't support Node < 16, the error message in tsc is very general:

Cannot find module '...' or its corresponding type declarations.

Same in VS Code:

Screenshot in VS Code of 'Cannot find module '@typescript-eslint/utils' or its corresponding type declarations. ts(2307)' in a hover box

Could we please have a secondary message added suggesting that the project be switched to using a more modern "moduleResolution"?

This message needs bikeshedding, but a starting proposal:

Cannot find module '...' or its corresponding type declarations.
    Consider upgrading your "moduleResolution" compiler option. See aka.ms/tsconfig#moduleResolution for details.

πŸ“ƒ Motivating Example

This has been a point of confusion in at least typescript-eslint/typescript-eslint#7284. We've gotten a few duplicate reports.

πŸ’» Use Cases

Same as ☝️, I think. I can't think of anything more specific.

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature labels Jul 21, 2023
@RyanCavanaugh
Copy link
Member

People are going to come here and complain if we issue that message and it turns out that the module really isn't there under any setting, and I don't really see a way we could determinatively detect which (if any) other resolution settings would work.

@JoshuaKGoldberg
Copy link
Contributor Author

Hmm. Maybe it can just link to aka.ms/tsconfig#moduleResolution then?

detect which (if any) other resolution settings would work

Also, I'm realizing now I meant to suggest in the OP that this message should only be there if any other resolution setting would work. 😬

@RyanCavanaugh
Copy link
Member

Under different module resolution settings, we could even have entirely different source files under analysis, rendering the original import moot. It's not tractable in the general case IMO

@DanielRosenwasser
Copy link
Member

Could we just have node10 do the same thing node16 does and specially handle the error where they would have diverged?

@DanielRosenwasser
Copy link
Member

Or something more naive where we keep track of whether exports was specified anywhere where we checked.

@blakeembrey
Copy link
Contributor

blakeembrey commented Oct 2, 2023

@DanielRosenwasser is it possible to use ambiguous language in the presence of β€œexports”? For example, β€œThis package may be resolvable using (…) because we found type module and exports declared in the package”?

I’ve only converted a few packages I maintain to be ESM so far, but every time I do I get issues like blakeembrey/change-case#304. The current message isn’t adequate to show the issue is in user configuration and not the package.

@DanielRosenwasser
Copy link
Member

We use "may" a lot of the time, so I'm okay with that.

@bradzacher
Copy link
Contributor

Coming back to this - this is a pretty persistent pain-point for users of @typescript-eslint.

We've found that a lot of users don't know that:

  • moduleResolution: node actually means node10
  • node16 exists
  • node10 is different to node16
  • they're using a module resolution that doesn't match their runtime (our minimum supported node version is 16).

We've had to keep this pinned issue up and open to give people docs on how to fix the unclear error message, or else they file an issue with us saying our package is broken.

I'm sure there are a lot of packages out there who continue to include a types field in their package.json solely to stave-off the support burden of users who are still (usually erroneously) using node10 resolution.
We could do this also - but it does seem "smelly" that we would consider changing our config to avoid support burden from an unclear TS error message.

I 110% get that we can't make the error message say something so absolute like "you're using node10 but this package doesn't have a types field declared, but it has an exports field, you must use node16 instead" because, well, it's a different module resolution and that advice may not even be correct!
But as @blakeembrey suggested maybe there's a way we can help nudge the user in the right direction for this specific case? Or at least suggest something that might help them try something else and self-service fix their problem, rather than having to reach for stackoverflow / github issues / discord messages in order to even learn about the 2 character config change that could fix their problem.

For example, would this rough logic be acceptable?
"if TS finds a package that has no types field but has an exports field with a nested types field, then append This package may be resolvable using (…) because we found typed exports declared in the package. to the error message"

@RyanCavanaugh
Copy link
Member

@andrewbranch does the above sound like something we can do?

@andrewbranch
Copy link
Member

Yes πŸ‘

@andrewbranch andrewbranch self-assigned this Nov 9, 2023
@andrewbranch andrewbranch added this to the TypeScript 5.4.0 milestone Nov 9, 2023
@andrewbranch andrewbranch added Experience Enhancement Noncontroversial enhancements and removed Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature labels Nov 9, 2023
@typescript-bot typescript-bot added the Fix Available A PR has been opened for this issue label Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Experience Enhancement Noncontroversial enhancements Fix Available A PR has been opened for this issue Suggestion An idea for TypeScript
Projects
None yet
7 participants