-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
separate typescript with types from typescript transformed #3118
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
5 Ignored Deployments
|
@@ -69,6 +69,7 @@ use crate::{ | |||
pub enum EcmascriptModuleAssetType { | |||
Ecmascript, | |||
Typescript, | |||
TypescriptSource, |
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.
Please add a comment explaining the difference between TS and TSSource. It seems TSSource is code that the dev is writing, where TS is code they're importing?
vec![if enable_typescript_transform { | ||
ModuleRuleEffect::ModuleType(ModuleType::Typescript(ts_transforms)) | ||
} else { | ||
ModuleRuleEffect::ModuleType(ModuleType::TypescriptSource(ts_transforms)) | ||
}], |
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 we're inside node_modules, shouldn't we disable type checking? Ie, only use TS, not TSSource.
vec![if enable_typescript_transform { | ||
ModuleRuleEffect::ModuleType(ModuleType::Typescript(ts_app_transforms)) | ||
} else { | ||
ModuleRuleEffect::ModuleType(ModuleType::TypescriptSource(ts_app_transforms)) | ||
}], |
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.
Opposite here, shouldn't this always be TSSource?
|
Benchmark for 301330fClick to view benchmark
|
| EcmascriptModuleAssetType::TypescriptDeclaration => true, | ||
EcmascriptModuleAssetType::Ecmascript => false, | ||
EcmascriptModuleAssetType::Typescript | EcmascriptModuleAssetType::Ecmascript => false, | ||
}; | ||
|
||
let parsed = parse(source, ty, transforms); |
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.
Nit: there are some references to is_typescript
in the code below that are not actually used, it's just a parameter that exists to be passed to other code that also doesn't use it and passes down.
Ok(BoolVc::cell( | ||
self.await?.resolve_options_context.await?.enable_types, | ||
context.enable_types && context.enable_typescript, |
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.
Maybe this should be an enum instead of 2 bools?
8fbb292
to
b535af2
Compare
b535af2
to
cb80040
Compare
Benchmark for 8f5b08e
Click to view full benchmark
|
…rbo#3118) fixes weird typescript errors caused by types being referenced by .ts files
…rbo#3118) fixes weird typescript errors caused by types being referenced by .ts files
fixes weird typescript errors caused by types being referenced by .ts files