GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Allow untyped imports · Pull Request #11446 · microsoft/TypeScript

Conversation

@ghost

@ghost Deleted user (ghost) commented Oct 7, 2016 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
SamVerschueren, sindresorhus, adamdbradley, danbucholtz, mlynch, jthoms1, queckezz, alex-kinokon, mhartington, tompiarulli, and Hotell reacted with hooray emoji
All reactions
  • 🎉 11 reactions
@ghost ghost assigned Vladimir Matveev (vladima) Oct 7, 2016
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Oct 7, 2016
Andy Hanson
Allow untyped imports
1807147
@ghost
ghost force-pushed the node_modules_package_json branch from db1cbaa to 1807147 Compare October 7, 2016 15:37
Andy Hanson
Add fourslash test
0170b71
@sheetalkamat

Sheetal Nandi (sheetalkamat) commented Oct 7, 2016

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Andy Hanson
Improve and increase test
28cfe54
@ghost

Deleted user (ghost) commented Oct 7, 2016 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author
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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ghost

Deleted user (ghost) commented Oct 7, 2016 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Oct 10, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.
SamVerschueren, mlynch, aluanhaddad, and Hotell reacted with thumbs up emoji
All reactions
  • 👍 4 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Andy Hanson
Allow to resolve to an untyped module even with --noImplicitAny, but …
12e45b2 
…error when checking it
@ghost

Deleted user (ghost) commented Oct 10, 2016 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Oct 11, 2016

Copy link
Copy Markdown
Member

Andy (Andrewkraft) (@Andy-MS) Nice - that one seems reasonable.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SamVerschueren

Sam Verschueren (SamVerschueren) commented Oct 11, 2016

Copy link
Copy Markdown

Nice work Andy (Andrewkraft) (@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 Daniel Rosenwasser (@DanielRosenwasser) suggested. The only difference is that I suggested an extra flag called noImplicitAnyModule. But using noImplicitAny might be even better!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mhegazy
Mohamed Hegazy (mhegazy) suggested changes Oct 11, 2016
View reviewed changes

@mhegazy Mohamed Hegazy (mhegazy) left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/checker.ts
resolvedModule.resolvedFileName);
}

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

@mhegazy Mohamed Hegazy (mhegazy) Oct 11, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mhegazy Mohamed Hegazy (mhegazy) Oct 11, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) reviewed Oct 13, 2016
View reviewed changes
Comment thread src/compiler/diagnosticMessages.json
"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.": {

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Oct 13, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Since -> Because

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) reviewed Oct 13, 2016
View reviewed changes
Comment thread src/compiler/moduleNameResolver.ts
moduleName: string, directory: string, failedLookupLocations: string[], state: ModuleResolutionState, checkOneLevel: boolean, typesOnly: boolean): ResolutionResult {
directory = normalizeSlashes(directory);
/**
* Remembers the path to a package.json for the package.

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Oct 13, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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."

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jthoms1 Josh Thomas (jthoms1) mentioned this pull request Oct 13, 2016
Wildcard module declaration hides all import errors ionic-team/ionic2-app-base#93
Closed
@ghost ghost mentioned this pull request Oct 18, 2016
Have module resolution be independend of --allowJs and --jsx flags #11704
Merged
@ghost ghost mentioned this pull request Oct 27, 2016
Allow untyped imports #11889
Merged
@ghost

Deleted user (ghost) commented Oct 27, 2016

Copy link
Copy Markdown
Author

Closing in favor of #11889
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ghost ghost closed this Oct 27, 2016
@ghost
ghost deleted the node_modules_package_json branch November 9, 2016 20:54
@microsoft Microsoft (microsoft) locked and limited conversation to collaborators Jun 19, 2018
@typescript-bot TypeScript Bot (typescript-bot) unassigned Vladimir Matveev (vladima) 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.

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Better handling of untyped node module imports

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@sheetalkamat @DanielRosenwasser @SamVerschueren @mhegazy @vladima @msftclas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.