Skip to content
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

Merged
merged 11 commits into from Jul 8, 2020
Merged

feat: add --no-check option #6456

merged 11 commits into from Jul 8, 2020

Conversation

@kitsonk
Copy link
Contributor

kitsonk commented Jun 24, 2020

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:

  • There aren't any tests yet, wanted to get any debate over functionality out of the way first.
  • I have enabled isolatedModules in the compiler.ts. This should try to catch things that like const enum which would be breaking when using the --no-check option, but... this subtly breaks lots of things... in isolatedModules any module that looks like a script breaks, which means that having to do export {} 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 use isolatedModules.
  • Code which does 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 as export 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, requiring export type to be used instead, but it would also error on import { 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.
  • There could be improvements to the module graph sent to the compiler, as we don't have to send all the dependencies like we have to with a type check. For example, sending JavaScript just doesn't make sense (unless 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 versus deno 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.

@kitsonk kitsonk changed the title Add --no-check option [WIP] Add --no-check option Jun 24, 2020
@kitsonk kitsonk marked this pull request as draft Jun 24, 2020
@nayeemrmn
Copy link
Contributor

nayeemrmn commented Jun 24, 2020

Can we call it --no-type-check for maximum explicitness? For example Node has --check referring to a syntax check.

@kitsonk kitsonk force-pushed the kitsonk:no-check branch from 6ca1405 to b994057 Jun 24, 2020
@ry
Copy link
Member

ry commented Jun 24, 2020

I'd prefer --no-check. I want to change the "Compiling ..." messages to "Checking ..." so I think --no-check plays nicely with that.

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.

@ry ry added this to the 1.2.0 milestone Jun 24, 2020
@kitsonk kitsonk force-pushed the kitsonk:no-check branch 3 times, most recently from f7e7672 to 03b3e77 Jun 25, 2020
@kitsonk
Copy link
Contributor Author

kitsonk commented Jun 28, 2020

Release compiler time on my laptop with the release build for https://deno.land/x/oak/examples/server.ts with a --reload:

  • check: 4.3 secs
  • no check: 2.6 secs

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.

@kitsonk kitsonk force-pushed the kitsonk:no-check branch 3 times, most recently from 2c0f695 to 10bc5a7 Jun 29, 2020
@kitsonk kitsonk changed the title [WIP] Add --no-check option feat: add --no-check option Jul 1, 2020
@kitsonk
Copy link
Contributor Author

kitsonk commented Jul 1, 2020

Ok, PR is ready for a proper review now.

The really outstanding problem is the challenge around using --no-check with code that works fine normally, but breaks under --no-check. When a module exports a type reference from another module, --no-check lacks the type information to know it has no runtime emit. That means the following breaks:

export { AnInterface } from "./mod.ts";

In order to get this to work export type must be used:

export type { AnInterface } from "./mod.ts";

This PR contains conversions of a lot of the type import/export code to use import type and export type to make transpilation easier. The problem is that the error message at the moment is not very informative to what the cause of the problem is:

error: Uncaught SyntaxError: The requested module './mod.ts' does not provide an export named 'AnInterface'

We may want to trap this error message, so when running under --no-check and trying to load a module, that we provide more information to indicate that the code needs to be changed to export type. There is an integration test (error_no_check) that validates the expected output.

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.

Ref: denoland/deno_website2#1246

@kitsonk kitsonk marked this pull request as ready for review Jul 1, 2020
@kitsonk
Copy link
Contributor Author

kitsonk commented Jul 1, 2020

One other UX thing worth considering... Because we have moved to Check ... when "compiling" TypeScript, there is no output when just transpiling. That is good except when doing deno test --no-check now just "hangs" there for a while while dealing with transpiling for a lot of code. (Let alone the output is still very confusing when doing test: Check file:///some/path/.deno.test.ts where some/path is only valid when no path is set on the command line)

It might be better to not output Check at all when doing a compile of tests, and instead output something like Test file:///some/path/passed/as/argument irrespective of the check/no-check status.

deno_typescript/lib.rs Show resolved Hide resolved
tools/benchmark.py Show resolved Hide resolved
@bartlomieju bartlomieju self-requested a review Jul 2, 2020
@kitsonk kitsonk force-pushed the kitsonk:no-check branch from 10bc5a7 to 1f05aa7 Jul 3, 2020
Copy link
Contributor

bartlomieju left a comment

Thanks @kitsonk! I'm very surprised that it's so little code to provide this feature. I have just a few minor comments

cli/js/compiler.ts Outdated Show resolved Hide resolved
cli/js/compiler.ts Outdated Show resolved Hide resolved
cli/tests/integration_tests.rs Show resolved Hide resolved
cli/tsc.rs Outdated Show resolved Hide resolved
cli/tsc.rs Show resolved Hide resolved
@ry
Copy link
Member

ry commented Jul 6, 2020

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?

@kitsonk
Copy link
Contributor Author

kitsonk commented Jul 6, 2020

@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 --no-check with a single file change against the incremental check as well, so for common workflows it might be faster.

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.

kitsonk added 2 commits Jun 24, 2020
kitsonk added 8 commits Jun 28, 2020
@kitsonk kitsonk force-pushed the kitsonk:no-check branch from 0de8263 to 7de6c04 Jul 7, 2020
@ry
ry approved these changes Jul 8, 2020
Copy link
Member

ry left a comment

LGTM - thanks @kitsonk !!

@bartlomieju please merge this when you approve. I'm not sure if you wanted to make any changes.

Copy link
Contributor

bartlomieju left a comment

LGTM, thank you @kitsonk!

Now let's figure out how to do it all in Rust 😋

@bartlomieju bartlomieju merged commit 82aabb6 into denoland:master Jul 8, 2020
7 checks passed
7 checks passed
test_release macOS-latest
Details
test_release windows-2019
Details
test_release ubuntu-16.04
Details
test_debug ubuntu-16.04
Details
bench ubuntu-16.04
Details
lint ubuntu-16.04
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.