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

Allow untyped imports #11446

Closed
wants to merge 4 commits into from
Closed

Allow untyped imports #11446

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 7, 2016

Fixes #11106

If we find a package.json while searching through node_modules, we will allow imports of that module to work in an untyped fashion.
This change will not affect users with --noImplicitAny turned on.
It also has no effect without --moduleResolution node. So we might not want to completely close the issue until we have solutions for other module systems.

We can't do find-all-references for untyped imports because that function uses symbol.declarations, which in this case is empty.

@ghost ghost force-pushed the node_modules_package_json branch from db1cbaa to 1807147 Compare October 7, 2016 15:37
@sheetalkamat
Copy link
Member

Can you please add these test cases so we have baselines later if behavior modifies

  • allowJS true and resolutuion happens to be untyped package
  • allowjs true and resolution happens to be .js file
  • allowjs false and there exists .js file file in the package.

@ghost
Copy link
Author

ghost commented Oct 7, 2016

What Where
allowJS true and resolution happens to be untyped package untypedModuleImport_allowJs.ts
allowjs true and resolution happens to be .js file moduleResolution_allowJs.ts
allowjs false and there exists .js file file in the package. untypedModuleImport.ts

@ghost
Copy link
Author

ghost commented Oct 7, 2016

I find it suspicious that in moduleResolution_allowJs.types, we have exports.default : any, but when we import it we get foo: { bar(): number }. But that is a JS typings issue.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Oct 10, 2016

I don't think we should just be reporting Cannot find module 'foo'. to users - clearly we found something but the user is going to get undesirable behavior for their workflow. The correct thing to do is to explicitly tell the user that the module was found but that its is untyped/its shape has type any. I'd do something like

The module '{0}' resolved to a JavaScript package, but is considered untyped.
  Consider adding type declarations for '{0}' or using the '--allowJs' compiler option.

@ghost
Copy link
Author

ghost commented Oct 10, 2016

I went with:

A package for '{0}' was found at '{1}', but is untyped. Since '--noImplicitAny' is enabled, this package must have a declaration.

--allowJs would also require people to also use --nodeModulesMaxDepth 1.

We could mention that option, but this error message will be seen very often by people who simply forget to install types. Maybe we could even recommend npm install @types/foo in the error message (though typings for it might not exist).

@DanielRosenwasser
Copy link
Member

@andy-ms Nice - that one seems reasonable.

@SamVerschueren
Copy link

Nice work @andy-ms ! I wrote a blogpost about this a couple of weeks ago about The one thing that keeps me from using TypeScript. This is exactly what I wanted! Also with the error message that @DanielRosenwasser suggested. The only difference is that I suggested an extra flag called noImplicitAnyModule. But using noImplicitAny might be even better!

Copy link
Contributor

@mhegazy mhegazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing the filtering in resolveModuleName, why not just return an additional piece of information on ResolvedModule call it candidateFile for instance, and that would be the location of the .js file(s) that were found during the search for the .d.ts /.ts file. In program.ts, if we found this set but no resolvedFileName is set on the ResolvedModule, but a candidateFile, then we know it is untyped.

resolvedModule.resolvedFileName);
}

const newSymbol = createSymbol(SymbolFlags.ValueModule, quotedName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happenes if you just return undefined? why do you need to create a symbol?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you must, i would create a singleton UnkownModuleSymbol and check for that in isUntypedModuleSymbol

"category": "Message",
"code": 6141
},
"A package for '{0}' was found at '{1}', but is untyped. Since '--noImplicitAny' is enabled, this package must have a declaration.": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since -> Because

directory = normalizeSlashes(directory);
/**
* Remembers the path to a package.json for the package.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"The path to a package.json in case we find a typed version of the package further up the spine."

@ghost
Copy link
Author

ghost commented Oct 27, 2016

Closing in favor of #11889

@ghost ghost closed this Oct 27, 2016
@ghost ghost deleted the node_modules_package_json branch November 9, 2016 20:54
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better handling of untyped node module imports
6 participants