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

Generate TS DeclarationInfo provider independent of transpilation #1073

Closed
DavidANeil opened this issue Aug 29, 2019 · 19 comments
Closed

Generate TS DeclarationInfo provider independent of transpilation #1073

DavidANeil opened this issue Aug 29, 2019 · 19 comments
Labels
Can Close? We will close this in 30 days if there is no further activity enhancement

Comments

@DavidANeil
Copy link
Contributor

DavidANeil commented Aug 29, 2019

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.

@pauldraper
Copy link

pauldraper commented Sep 2, 2019

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:

  1. Declarations/type checking: --emitDeclarationOnly
  2. JS: --isolatedModules

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.

@DavidANeil
Copy link
Contributor Author

Yes, I assumed this would be implemented by having ts_library produce DeclarationInfo by running tsc with "emitDeclarationOnly": true, and then aspects could provide their JS either by doing a "declarations": false, "isolatedModules": true compile over the entire library, or by a script that calls ts.transpileModule, which would also give them control over whether they want a single action that transpiles the entire library, or 1 action per source file.

@DavidANeil
Copy link
Contributor Author

@gregmagolan @alexeagle This would be a fairly extensive breaking API change, so I think it should come before 1.0. What are your thoughts?

@DavidANeil
Copy link
Contributor Author

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.

@alexeagle
Copy link
Collaborator

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.

@evmar
Copy link
Contributor

evmar commented Sep 11, 2019

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.)

@DavidANeil
Copy link
Contributor Author

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 jobs=12
Waiting for type checking has never been an option for us and we already circumvent this ruleset's devmode steps and transpile each file as its own action using tsickle. It would be nice if we didn't have to actively maintain a patch that produces the .d.ts files from the prodmode step though.

We haven't succeeded in patching rules_angular yet, so we currently transpile all of our Angular code 4 times, and typecheck it twice, which would be nice to knock down to just the 3 transpilations we actually use and 1 typecheck.

Hopefully if rules_nodejs set a better pattern, Angular would update to match.

@evmar
Copy link
Contributor

evmar commented Sep 11, 2019

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.

@DavidANeil
Copy link
Contributor Author

DavidANeil commented Sep 12, 2019

For a ts_library with 15 source files, and no deps:

INFO: Elapsed time: 10.359s, Critical Path: 8.59s
INFO: 1 process: 1 linux-sandbox.

Just for the sake of fun numbers...
ng_module with ~1700 source ts files (+ ~300 html) and lots of deps:

INFO: Elapsed time: 65.732s, Critical Path: 64.49s
INFO: 1 process: 1 linux-sandbox.

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)

ts_library with 1905 source files, and lots of deps:

INFO: Elapsed time: 81.505s, Critical Path: 80.45s
INFO: 1 process: 1 linux-sandbox.

I can't even run tsc on a single TS file and have it take less than a second.

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.

@alexeagle
Copy link
Collaborator

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.
(I notice that you're not using a worker to run the ts_library action, it would help a bit in caching the bound source files, and possibly be able to re-use the oldProgram so it could behave like tsc --watch. Note that lowering the number of workers to 1 ensures that you get a cache hit on the oldProgram)

Angular itself has a similar problem, @angular/core/src is approx. one big ts_library and it slows the Angular team down.

We're discussing ts_auto_deps or some better solution for BUILD file generation that would help you create/maintain a tree of ts_library roughly per-directory. But of course that only works for non-cyclic partitions of the tree.

@DavidANeil
Copy link
Contributor Author

Last month I split our dependency graph up a lot (we went from ~250 ts_library|ng_module to ~800) and all it did was increase type checking from ~30 minutes to ~3 hours.
I had to revert the change.

@alexeagle
Copy link
Collaborator

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 bazel build --profile output showing what was the critical path?

@DavidANeil
Copy link
Contributor Author

We can't use workers because of #757
Would you like the critical path from when we had smaller packages, or the current one?

@alexeagle
Copy link
Collaborator

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

@pauldraper
Copy link

pauldraper commented Jul 14, 2020

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.

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.

@alexeagle
Copy link
Collaborator

ts_library is quite tied to Google usage and harder to change due to dependency on google3-Source of Truth.
I have the same requirement to run the app regardless of typecheck errors at my current job so we'll either use Babel or ts_project

@alexeagle
Copy link
Collaborator

Note, I'm using ts_project for this now, just fixed related #2116 to make typecheck-only ts_project easier to use

@github-actions
Copy link

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!

@github-actions github-actions bot added the Can Close? We will close this in 30 days if there is no further activity label Nov 18, 2020
@github-actions
Copy link

github-actions bot commented Dec 2, 2020

This issue was automatically closed because it went two weeks without a reply since it was labeled "Can Close?"

@github-actions github-actions bot closed this as completed Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Can Close? We will close this in 30 days if there is no further activity enhancement
Projects
None yet
Development

No branches or pull requests

4 participants