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.

Sign up for GitHub

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

Jump to bottom

Cache invalidation when recompiling a module #1692

Closed
kitsonk opened this issue Feb 5, 2019 · 2 comments · Fixed by #5817
Closed

Cache invalidation when recompiling a module #1692

kitsonk opened this issue Feb 5, 2019 · 2 comments · Fixed by #5817
Assignees
@bartlomieju
Labels
bug Something isn't working correctly high priority

Comments

@kitsonk
Copy link
Contributor

kitsonk commented Feb 5, 2019

In discussion of a StackOverflow question it reminded me have a situation we don't handle well, which could result in runtime errors (or unexpected behaviour).

The scenario is as follows:

  • There are two modules a.ts and b.ts
  • b.ts exports a value that is consumed by a.ts, which is a "main" module
  • Deno is run as deno a.ts
    • Both a.ts and b.ts are transpiled and cached
    • a.js and b.js are injected into the runtime
  • Changes are made to b.ts that remove the export
  • Deno is run as deno a.ts again
    • Deno invalidates the cache of b.ts and recompiles it
    • a.js and b.js are injected into the runtime
    • a.js fails at runtime because the export is no longer available in b.js

We need some sort of reverse dependency graph that we would need to persist along with the cached outputs, so we could determine if b.ts is invalidated that any modules that depend upon b.ts should also be invalidated.

This is a longterm issue to fix, not something that can be easily addressed, we still have some work to do in cleaning up the compiler and how we deal with modules, and how we cache modules before we would want to tackle this. It is just a reminder to not forget it.
@ry
Copy link
Member

ry commented Feb 5, 2019

Thank you for the clear example. I've also had the sneaking suspicion that we had this bug but hadn't actually run into it nor took the time to think it through properly.

It would be great to get a failing integration test for this (which might be somewhat complicated).

I do think we need to be writing "depfiles" for each generated file (I can't find a link describing what a depfile is but it's essentially a simplified makefile)...

@bartlomieju bartlomieju mentioned this issue Sep 14, 2019
Closed
@ry ry mentioned this issue Nov 13, 2019
Closed
@bartlomieju bartlomieju self-assigned this Feb 24, 2020
@bartlomieju bartlomieju added the bug Something isn't working correctly label Feb 24, 2020
@mzdunek93
Copy link
Contributor

TypeScript compilation can and needs to be optimized significantly. The issue is that currently, after a change of file in the dependency tree, it's recompiled along with all of its descendants in the tree, which is completely unnecessary - its ancestors, the files that depend on the modified one, should be the one to be recompiled. In order to mitigate that, we can use the TS compiler's option to perform incremental compilation - that way it saves a build info file with all the dependencies and their hashes, and the next time it only recompiles the necessary files. For this to work correctly, we have to always start compilation from the same file, instead of skipping the root if it's already in the cache. I have a prototype working locally, if anyone's interested I can improve it and make a PR.

@mzdunek93 mzdunek93 mentioned this issue May 5, 2020
Closed
@bartlomieju bartlomieju mentioned this issue May 25, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bartlomieju bartlomieju

Labels
bug Something isn't working correctly high priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor: TS compiler and module graph bartlomieju/deno
4 participants
@ry @kitsonk @mzdunek93 @bartlomieju