-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Fix crash when checking module exports for export= #10538
Conversation
Also make maxNodeModuleJsDepth default to 0 so that incorrect tsconfigs now let the compiler spend less time compiling JS that is found in node_modules (especially since most people will already have the d.ts and want ignore the JS anyway). jsconfig still defaults to 2.
@andy-ms and @vladima can you take a look? |
@mhegazy you should also know about this fix. |
// @traceResolution: true | ||
// @noEmit: true | ||
|
||
// @filename: c:/root/tsconfig.json |
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.
I would replace c:/root/
with just /
.
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.
fixed
👍 |
/// <reference path="c:/root/typings/index.d.ts" /> | ||
import * as foo from "shortid"; | ||
foo.x // found in index.d.ts | ||
foo.y // ignored from shortid/index.ts |
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.
shortid/index.js
and not shortid/index.ts
.
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.
fixed
can you add the same test with maxNodeModuleJsDepth == 1 and then the error message should be on |
👍 |
Actually if there is a d.ts, then it is always used. There is no way to get the compiler to prefer |
@@ -4,7 +4,7 @@ | |||
// @traceResolution: true | |||
// @noEmit: true | |||
|
|||
// @filename: c:/root/tsconfig.json | |||
// @filename: tsconfig.json |
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.
You need to include the /
in front or else your own directory will show up in the trace.
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.
good to know, I didn't notice that. fixed.
Travis is down, but all tests pass when I merge master back into the branch, so I'll go ahead and merge. @yuit |
Fixes #10460 and fixes #10522
This PR also makes maxNodeModuleJsDepth default to 0 so that incorrect tsconfigs now let the compiler spend less time compiling JS that is found in node_modules (especially since most people will already have the d.ts and want ignore the JS anyway).
jsconfig still defaults to 2.