-
Notifications
You must be signed in to change notification settings - Fork 522
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
Generate TS DeclarationInfo provider independent of transpilation #1073
Comments
It's been my opinion of the inverse, that consumers of the JS should not have to wait for declarations/type checking. TypeScript has the deliberate goal that type checking and transpilation can be easily separated.* This is a phenominal property, but currently rules_typescript is not leveraging it. There should be two actions:
In dev mode, you could choose to run just the second and have extremely fast iteration, regardless of target count, size, or structure. And then use an IDE or manual invocation to check types when desired. (Some users would of course prefer to always both type check and run as it does today.) Or as said earlier, typescript libraries could be type checked without generating the output of their transitive dependencies. I think tsickle may violate that goal. Also I would not be surprised if ngc found a way to break that as well. |
Yes, I assumed this would be implemented by having |
@gregmagolan @alexeagle This would be a fairly extensive breaking API change, so I think it should come before 1.0. What are your thoughts? |
Proposals 2 and 3 were superflous so I have edited the main post to focus only on not doing transpilation as part of type-checking and not doing type-checking as part of transpilation. |
We have discussed this internally as well. Yes, it's true that type-checking could be a separate action. We think there are some performance benefits but haven't quantified them. /cc @evmar We would want to make such a change under both Blaze (google3) and Bazel at the same time so 1.0 timeframe isn't reasonable. We'll have to revisit early next year. |
Our experience with compilation at Google is that in a world in which you can run the app before type checking, you end up in a pattern where you write code until the tests pass, then later come through and try to make the types work out, and the resulting types then are just "whatever makes the compiler accept the code". Because of this I think we (Google) are not interested in running the code before the typecheck, but you can make the argument that external users have different requirements. You are right that you can get the code to a runnable state faster if you don't involve type checking. The approach we've taken so far with TS is to just make type checking fast enough that people don't mind it. (In fact, exactly because we historically made code runnable without type checking in the past is the reason our type checker got so slow as to be unusable -- nobody cared about performance enough because they didn't run it very often.) (I'm not sure if prodmode is still Closure JS in rules_node, but at least Closure JS generation requires type info from TS, so it cannot be produced without first running type checking. And (as you guessed) ngc also requires type info.) |
If I had to give an estimate I would say if I changed a file that is somewhere near the middle of our dependency graph then I would be looking at around a 20 minute wait for type checking to complete with We haven't succeeded in patching Hopefully if |
I don't know a lot about either public bazel or Angular, but at least within Google across all our apps (including the enormous ones), our median compilation time to recompile a ts_library is around 1s. |
For a
Just for the sake of fun numbers...
Though that one often takes over 10 minutes on CI for ... reasons. (I suspect it is struggling to upload the ~10000 generated files to the cache)
I can't even run Though this is all tangential to this issue, it would be nice if we could speed up type checking such that we didn't need them to be done separately. |
As you know, the fundamental property you're missing is a fine-grained dependency graph of smallish typescript libraries. We never designed for libraries so massive. Angular itself has a similar problem, We're discussing |
Last month I split our dependency graph up a lot (we went from ~250 |
Hmm we should drill into that a bit more. We have apps at Google at a similar scale, so I wonder if something is misconfigured or not well documented. Did you have the worker strategy enabled? Can you share a |
We can't use workers because of #757 |
Oops!! #757 is easy to fix. I added it to our Customer Support hotlist so we'll do it soon - let's not investigate until you can enable it |
That argument, in fact, is made by the language itself. It is not an accident that TypeScript can compile without types. It is a fundamental design requirement of language which its authors uphold at all costs. There are any number of cool features that are not implemented because it would break this requirement. After so much design effort, it's disappointing when you can't actually leverage it. Separate .js and .d.ts outputs ought to be the default mode of operation, or at the very least an option. |
ts_library is quite tied to Google usage and harder to change due to dependency on google3-Source of Truth. |
Note, I'm using |
This issue has been automatically marked as stale because it has not had any activity for 90 days. It will be closed if no further activity occurs in two weeks. Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs! |
This issue was automatically closed because it went two weeks without a reply since it was labeled "Can Close?" |
Currently, any target that depends on the
.d.ts
files of another target is forced to run the devmode transpilation action, even if they don't need to consume the actual JavaScript output.Conversely, any target that depends on either the prodmode or devmode js also must run type checking (though only devmode will generate
.d.ts
files.)These are very different concerns and so should be produced by separate actions.
The text was updated successfully, but these errors were encountered: