-
Notifications
You must be signed in to change notification settings - Fork 5.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: unexpected skip of emit in TS compiler and double compilation of dynamic imports #6177
Conversation
cli/js/compiler.ts
Outdated
// If `checkJs` is off we still might be compiling entry point JavaScript file | ||
// (if it has `.ts` imports), but it won't be emitted. | ||
if (options.checkJs) { | ||
assert( | ||
emitResult.emitSkipped === false, | ||
"Unexpected skip of the emit." | ||
); | ||
} |
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.
This assertion is only half a fix. @kitsonk I'm gonna need your advice here. Let me describe what's was going on pre-1.0.3 and what's going on past-1.0.3.
pre-1.0.3
If an entry point was a .js
file that had .ts
imports then TS compiler worker was spawned for each import.
All dependencies from the .ts
import were transpiled and cached to disk (this led to situation were the same .ts
file might have been transpiled and cached multiple times in a single run).
post-1.0.3
If an entry point is a .js
file that has .ts
imports then TS compiler worker is created once and starts compilation with the .js
entry point. If checkJs
setting is not passed then I have found that TS compiler doesn't check and emit .ts
imports. I guess that makes sense, but... I do want to start TS compiler with .js
entry point and I do want it to check and transpile only .ts
source files. @kitsonk is there a compiler option we could use to make it behave that way?
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 guess I'm talking about changing default value of allowJs
to true
...
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 see about changing. I think this is related problem.
#6176
Will it be a restriction for JavaScript with TypeScript on Deno?
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.
Sorry, yes, allowJs
needs to be on in those situations. The problem with it being on all the time is that is goes and loads huge bits of JavaScript and parses them, even with checkJs false. So I think if the dep graph has some sort of ".js loading .ts" when we should turn it on, but only then... if that ".ts loading .js that only loads other .js" should be off.
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.
Hard to totally grok in isolation, but looks like the right direction.
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.
LGTM
Fixes #6082
Fixes #5982
Fixes #6057
Fixes #6144