-
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
feat: add --no-check option #6456
Conversation
Can we call it |
I'd prefer Thanks for this work @kitsonk, this has been a long time coming - glad to see it. I'll wait to land this until 1.2.0 (July 13). #6428 can land first since it is not introducing any interface changes. |
f7e7672
to
03b3e77
Compare
Release compiler time on my laptop with the release build for
I need to do some further investigations where one file is changed in the tree, but both the incremental and the no checking should help speed that up. |
2c0f695
to
10bc5a7
Compare
Ok, PR is ready for a proper review now. The really outstanding problem is the challenge around using export { AnInterface } from "./mod.ts"; In order to get this to work export type { AnInterface } from "./mod.ts"; This PR contains conversions of a lot of the type import/export code to use
We may want to trap this error message, so when running under This also contains a benchmark test that compares a 20 module workload under check and no-check and there is a related PR on the website to expose the information in a graph, so we can track performance of check and no-check over time. |
One other UX thing worth considering... Because we have moved to It might be better to not output |
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.
Thanks @kitsonk! I'm very surprised that it's so little code to provide this feature. I have just a few minor comments
It's unfortunate that the speed up isn't more. Is it possible tsc isn't being invoked in the most optimal way? Or is any operation in tsc just hopelessly slow? |
@ry there is no more optimal way. I will try to tease out better where we are spending the time as a percentage. I also haven't compared a My anecdotal evidence is we are saving about 95-100% of the type checking time that tsc was spending on things, so the rest is effectively the AST parse and emitting. In the end, we never wanted this in the compiler, we want it in swc. This is just a step in the right direction. Also seeing how relatively "slow" the parse is in isolation, means we should also keep moving forward with the AST parse in Rust. |
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 - thanks @kitsonk !!
@bartlomieju please merge this when you approve. I'm not sure if you wanted to make any changes.
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, thank you @kitsonk!
Now let's figure out how to do it all in Rust 😋
This commit adds a "--no-check" option to following subcommands: - "deno cache" - "deno info" - "deno run" - "deno test" The "--no-check" options allows to skip type checking step and instead directly transpiles TS sources to JS sources. This solution uses `ts.transpileModule()` API and is just an interim solution before implementing it fully in Rust.
This commit adds a "--no-check" option to following subcommands: - "deno cache" - "deno info" - "deno run" - "deno test" The "--no-check" options allows to skip type checking step and instead directly transpiles TS sources to JS sources. This solution uses `ts.transpileModule()` API and is just an interim solution before implementing it fully in Rust.
This commit adds a "--no-check" option to following subcommands: - "deno cache" - "deno info" - "deno run" - "deno test" The "--no-check" options allows to skip type checking step and instead directly transpiles TS sources to JS sources. This solution uses `ts.transpileModule()` API and is just an interim solution before implementing it fully in Rust.
This commit adds a "--no-check" option to following subcommands: - "deno cache" - "deno info" - "deno run" - "deno test" The "--no-check" options allows to skip type checking step and instead directly transpiles TS sources to JS sources. This solution uses `ts.transpileModule()` API and is just an interim solution before implementing it fully in Rust.
This commit adds a "--no-check" option to following subcommands: - "deno cache" - "deno info" - "deno run" - "deno test" The "--no-check" options allows to skip type checking step and instead directly transpiles TS sources to JS sources. This solution uses `ts.transpileModule()` API and is just an interim solution before implementing it fully in Rust.
This commit adds a "--no-check" option to following subcommands: - "deno cache" - "deno info" - "deno run" - "deno test" The "--no-check" options allows to skip type checking step and instead directly transpiles TS sources to JS sources. This solution uses `ts.transpileModule()` API and is just an interim solution before implementing it fully in Rust.
This commit adds a "--no-check" option to following subcommands: - "deno cache" - "deno info" - "deno run" - "deno test" The "--no-check" options allows to skip type checking step and instead directly transpiles TS sources to JS sources. This solution uses `ts.transpileModule()` API and is just an interim solution before implementing it fully in Rust.
This commit adds a "--no-check" option to following subcommands: - "deno cache" - "deno info" - "deno run" - "deno test" The "--no-check" options allows to skip type checking step and instead directly transpiles TS sources to JS sources. This solution uses `ts.transpileModule()` API and is just an interim solution before implementing it fully in Rust.
This commit adds a "--no-check" option to following subcommands: - "deno cache" - "deno info" - "deno run" - "deno test" The "--no-check" options allows to skip type checking step and instead directly transpiles TS sources to JS sources. This solution uses `ts.transpileModule()` API and is just an interim solution before implementing it fully in Rust.
Resolves #5436
This PR adds a
--no-check
option to relative subcommands and adds a request to the compiler of transpile, which takes the module graph and transpiles each module.There are a few things that I think still need to be addressed:
I have enabledthis subtly breaks lots of things... inisolatedModules
in thecompiler.ts
. This should try to catch things that likeconst enum
which would be breaking when using the--no-check
option, but...isolatedModules
any module that looks like a script breaks, which means that having to doexport {}
in a module that doesn't have any imports or exports. Until TypeScript supports some way of asserting a module that doesn't look like a module, we can't really useisolatedModules
.export { AnInterface } from "mod.ts"
breaks under--no-check
because the compiler doesn't know to erase it, because it doesn't know it is a type only export. These need to be written asexport type { AnInterface } from "mod.ts"
. The compiler option"importsNotUsedAsValues"
being set to"error"
could help improve the situation, as it would error during a "normal" compile, requiringexport type
to be used instead, but it would also error onimport { AnInterface } from "mod.ts"
. This would also likely break a lot of existing code. The net result is that a--no-check
will produce errors like this:error: Uncaught SyntaxError: The requested module 'https://deno.land/std@0.58.0/ws/mod.ts' does not provide an export named 'WebSocket'
. One action that we could do is make that error more informative, like provide the requesting module. This commit is what I needed to do to oak to get it to be supportable under--no-check
.checkJs
is true which flags we want transpiling) as well as.d.ts
files shouldn't be sent (currently, they are returned as blank to appease the logic which processes the result of the module graph).So the release build, running
deno cache -r https://deno.land/x/oak/examples/server.ts
is taking about ~10s versusdeno cache -r --no-cache https://deno.land/x/oak/examples/server.ts
is taking ~5s, including the time to download all the modules. So that is a pretty dramatic improvement. Still would be nice to get the ~5s shorter though.