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

Give better error messages for bare imports with badly configured projects #35876

Closed
DanielRosenwasser opened this issue Dec 27, 2019 · 16 comments · Fixed by #38105
Closed

Give better error messages for bare imports with badly configured projects #35876

DanielRosenwasser opened this issue Dec 27, 2019 · 16 comments · Fixed by #38105
Assignees
Labels
Experience Enhancement Noncontroversial enhancements Fix Available A PR has been opened for this issue Fixed A PR has been merged for this issue Good First Issue Well scoped, documented and has the green light Help Wanted You can do this Rescheduled This issue was previously scheduled to an earlier milestone Suggestion An idea for TypeScript

Comments

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Dec 27, 2019

When targeting es2015 or later, moduleResolution isn't set to node by default.

Perhaps when

  • moduleResolution is calculated to be classic and
  • paths aren't specified

we should hint to the user

Did you mean to set the 'moduleResolution' option to 'node', or to add aliases to the 'paths' option?
@danielkcz
Copy link

danielkcz commented Jan 9, 2020

I want to add here, that the Playground does not even allow to set a moduleResolution.

I tried reproducing some errors and was confused about why am I getting different outcomes in the playground compared to my actual code.

Only after fiddling with it CodeSandbox I eventually found out that moduleResolution: node did the trick.

I was not able to find if Playground issues should be reported somewhere else. Should I open a separate issue here?

Why is the classic resolution still being kept as default for a new module types?

@DanielRosenwasser
Copy link
Member Author

Yes, https://github.com/Microsoft/TypeScript-Website, though I would say the website should probably just always display moduleResolution: node since anything else is useless.

Why is the classic resolution still being kept as default for a new module types?

Yeah, there's #11434 but I don't think we ever got around to the work.

@RyanCavanaugh RyanCavanaugh added Experience Enhancement Noncontroversial enhancements Suggestion An idea for TypeScript labels Jan 14, 2020
@DanielRosenwasser DanielRosenwasser added Good First Issue Well scoped, documented and has the green light Help Wanted You can do this labels Feb 26, 2020
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 3.9.0 milestone Feb 26, 2020
@orta
Copy link
Contributor

orta commented Feb 27, 2020

The playground side of this is done (it always defaults to moduleResolution: node. )

Which leaves the warning on imports to do 👍

@mshivaku99
Copy link
Contributor

Can me and @nityatammana work on this?

@DanielRosenwasser
Copy link
Member Author

Sure - you might want to create one new diagnostic in diagnosticMessages.json

Cannot find module '{0}'. Did you mean to set the 'moduleResolution' option to 'node', or to add aliases to the 'paths' option?

Run npx gulp generate-diagnostics

Run the tests

npx gulp runtests-parallel

see what breaks. Diff the folders tests/baselines/local and tests/baselines/reference with a tool like WinMerge or Beyond Compare to see if the new errors are good.

If nothing fails, you need to add a test case in a file named tests/cases/compiler/errorForBareSpecifierWithImplicitModuleResolution1.ts:

// @module: es2015

import { thing } from "non-existent-module";
thing()

Then run the tests to get baselines from that. Accept the baselines:

npx gulp baseline-accept

@mshivaku99
Copy link
Contributor

Hi again! To check in we've added the diagnostic and ran npx gulp generate diagnostics and npx runtests-parallel (on Windows through WSL). This was successful and it seemed as if the whole test suite passed. However, when diff-ing the folders the local folder just had the directories and subdirectories but no files.

Local Folder
image

Reference Folder
image

We're confused as to how to populate the local folder beyond running the tests locally.

Also, to clarify, in addition to adding to diagnosticMessages.json and adding test cases, do we need to throw the error in the source code?

@orta
Copy link
Contributor

orta commented Apr 6, 2020

The local folder only changes when the baselines have changed - so if your diffs don't match the reference folder.

Adding the throw in the user's project (e.g. creating the error diagnostic) should be the thing which will break a reference test of two hopefully.

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Apr 6, 2020

Let's steer away from the word "throw" because that's just not the way TypeScript reports errors.

You need to report the diagnostic/error that I mentioned in my last comment instead of the current error when the project is misconfigured.

@mshivaku99
Copy link
Contributor

Me and @nityatammana think we've found a fix, but after accepting the baselines, 10 tests fail that don't include tests we've modified baselines for and don't appear in the tests/baselines/local folder after failing. I've attached images of the first 3 below.

image

image

image

If anyone has any insights, I would be very happy to hear it!

@sheetalkamat
Copy link
Member

You need to edit the error on src\testRunner\unittests\tsserver\symLinks.ts

@tangobravo
Copy link

If the default isn't changed to 'node' everywhere (to maintain backwards compatibility) - could a warning just be emitted whenever 'moduleResolution' defaults to classic (rather than just when an import fails).

Something like "warning: moduleResolution defaulting to classic due to your module setting, which is probably not what you want."

If the moduleResolution is explicitly set to classic (either in tsconfig or cmd line) then that would silence the warning.

I'd personally favour just changing the default, but as it would be a behaviour change for existing projects then I understand why it might not have happened yet.

@typescript-bot typescript-bot added the Fix Available A PR has been opened for this issue label Jul 30, 2020
@DanielRosenwasser
Copy link
Member Author

Thanks for the PR @mshivaku99, and thank you @orta for helping guide the PR!

@DanielRosenwasser DanielRosenwasser added the Fixed A PR has been merged for this issue label Jul 30, 2020
@DanielRosenwasser
Copy link
Member Author

(and @sheetalkamat for the guidance above!)

@jernchr11
Copy link

I ran into this issue also. Why isn't "node" the default module resolution strategy?

@saramorsi
Copy link

How do I fix the issue? I'm running into the same issue in my project. Thanks!

@jernchr11
Copy link

jernchr11 commented Apr 9, 2021

@saramorsi , I just added this line under compilerOptions in my tsconfig.json file and it solved my problem.

"moduleResolution": "node"

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 Fixed A PR has been merged for this issue Good First Issue Well scoped, documented and has the green light Help Wanted You can do this Rescheduled This issue was previously scheduled to an earlier milestone Suggestion An idea for TypeScript
Projects
None yet
10 participants