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

Add assumeChangesOnlyAffectDirectDependencies as a option to skip checking and generating .d.ts files for files indirectly importing affected file #35711

Merged
merged 6 commits into from Jan 3, 2020

Conversation

sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented Dec 16, 2019

Fixes #33329 by providing this flag to allow gulp-tsb like build

cc: @mjbvz

src/compiler/commandLineParser.ts Outdated Show resolved Hide resolved
src/compiler/diagnosticMessages.json Outdated Show resolved Hide resolved
@mjbvz
Copy link
Contributor

@mjbvz mjbvz commented Dec 16, 2019

@sheetalkamat Very nice. Just tested this using the test case from #33329 and confirmed that recompile time is now very similar to gulp-tsb

/cc @jrieken

@sheetalkamat
Copy link
Member Author

@sheetalkamat sheetalkamat commented Dec 18, 2019

@RyanCavanaugh @DanielRosenwasser Do you think the name of the flag is ok or otherwise please suggest something. Note that its not just check that gets disabled but also ".d.ts" emit of these indirectly importing files.

@sheetalkamat sheetalkamat changed the base branch from baselineTscWatch to master Dec 18, 2019
@andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Dec 18, 2019

skipIndirectImportProcessing?

In an incremental build, skip type checking and .d.ts file generation for files indirectly importing changed files.

?

@DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Dec 18, 2019

assumeSemanticChangesOnlyImpactDirectDependencies

@DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Dec 18, 2019

You left a laugh reaction, but I'm like half serious

@andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Dec 18, 2019

That’s perfect; I’m only half laughing 👍

@RyanCavanaugh
Copy link
Member

@RyanCavanaugh RyanCavanaugh commented Dec 20, 2019

  • assumeSemanticChangesOnlyImpactDirectDependencies
  • skipIndirectImportProcessing
  • yolo
  • assumeChangesDoNotImpactIndirectDependencies
  • assumeChangesOnlyAffectDirectDependencies 👈

@weswigham
Copy link
Member

@weswigham weswigham commented Dec 20, 2019

assumeApproximateDependencies?

@sheetalkamat sheetalkamat changed the title Add noIndirectImports as a option to skip checking and generating .d.ts files for files indirectly importing affected file Add assumeChangesOnlyAffectDirectDependencies as a option to skip checking and generating .d.ts files for files indirectly importing affected file Jan 2, 2020
@@ -433,7 +433,9 @@ namespace ts {
return;
}

forEachReferencingModulesOfExportOfAffectedFile(state, affectedFile, (state, path) => handleDtsMayChangeOf(state, path, cancellationToken, computeHash));
if (!state.compilerOptions.assumeChangesOnlyAffectDirectDependencies) {
Copy link
Member

@amcasey amcasey Jan 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To someone unfamiliar with the builder, it's not clear how adding this simple check does what it says in the description. Where are the direct dependencies handled if not in the call below? At what point is the list expanded to include indirect dependencies?

Copy link
Member Author

@sheetalkamat sheetalkamat Jan 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getAffectedFiles handle the direct dependency and the below function handles indirect ones.

affectsSemanticDiagnostics: true,
affectsEmit: true,
category: Diagnostics.Advanced_Options,
description: Diagnostics.Have_recompiles_in_incremental_and_watch_assume_that_changes_within_a_file_will_only_affect_files_directly_depending_on_it
Copy link
Member

@amcasey amcasey Jan 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this is probably a very ignorant question, but does this apply only within projects or across projects as well? Clearly we can't get everything into the name, but it might be nice to provide more details in the description (or in a a comment, if we'd prefer to keep the switch mysterious and little-used).

Copy link
Member Author

@sheetalkamat sheetalkamat Jan 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builder is not used across projects but within single program.

@sheetalkamat sheetalkamat merged commit 18269c0 into master Jan 3, 2020
8 checks passed
@sheetalkamat sheetalkamat deleted the noIndirectImports branch Jan 3, 2020
@uniqueiniquity
Copy link
Contributor

@uniqueiniquity uniqueiniquity commented Jan 3, 2020

@sheetalkamat this PR added at least one test that causes long-path problems on git for windows. Could you try to shorten it?

error: unable to create file tests/baselines/reference/tscWatch/emitAndErrorUpdates/config-with---assumeChangesOnlyAffectDirectDependencies-and---declaration/updates-errors-when-file-transitively-exported-file-changes/when-there-are-circular-import-and-exports.js

There is a flag for git that might resolve the issue, but it seems better to just rename the test.

@mathias999us
Copy link

@mathias999us mathias999us commented Feb 8, 2020

I'm probably misunderstanding the point of assumeChangesOnlyAffectDirectDependencies, but I'm testing it on a large project on 3.8RC, and it doesn't seem to have any effect. For historical reasons, our project does not use modules, just namespaces at the file level. With the assumeChangesOnlyAffectDirectDependencies option on (and incremental), I make a test change in a file that is referenced (as far as I can tell) in a three other files. However, I see most files in the project are built, and it's still taking me 20 seconds to build our project when a small change is made.

Thanks for all your hard work on this great project. Happy to provide any other diagnostics / output that would be of help.

@DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Feb 10, 2020

Unfortunately that's as expected - codebases with global files don't have an explicit dependency graph, and every file is considered a direct dependency of a global.

@lensbart
Copy link

@lensbart lensbart commented Apr 16, 2020

@DanielRosenwasser we’re having the same issue as @mathias999us, but I don’t understand your comment. Can you clarify what’s the difference between codebases with and without global files?

Previously we specified an array of files in the files property in tsconfig. But after getting rid of this array, the problem still persists and assumeChangesOnlyAffectDirectDependencies doesn’t seem to do anything, neither as a compiler option in tsconfig, nor as a CLI flag.

What could we be doing wrong?

Many thanks (and apologies if this is not the right place to ask)!

@DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Apr 16, 2020

If your files are all global, TypeScript assumes that they are all compose into one big ball of globals that can impact each other in arbitrary ways. Regardless of any sort of arrangement based on file order or /// <reference path="..." /> directives, globals cause non-local errors and changes in types on every file in your project, so there's no assumptions we can make based on efficiency and independent checking.

For example, imagine 3 files

// a.ts
var x: string;
// b.ts
var x: string;
// c.ts
var c: string;

Now imagine another global file comes along

// d.ts
var x: number;

That declaration of x conflicts with other declaration of x. To model that, every single global file is effectively a direct dependency of every other global file.

image

Or, as a more 3D diagram might illustrate

image

My recommendation is to move off of global code (which our team is in the process of at #35561)

@mathias999us
Copy link

@mathias999us mathias999us commented Apr 17, 2020

@DanielRosenwasser Thanks for that straight-forward (and comical) explanation. However, our situation is still slightly different than the example you gave. We house all our code in namespaces at the file level (old-style TypeScript modules). I would think the TypeScript compiler should be able to determine that it only needs to compile other files that reference the namespace whose content is being changed.

@DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Apr 17, 2020

Namespaces are really just declarations that sit in the global scope - in theory we could do more work to say that when all files contain exactly one namespace, we can synthesize the dependency graph after type-checking and try to reuse it between compiles - but that's actually very different from the logic we use to establish dependencies between files today and would be a lot of work. My gut says that a better way forward is to migrate to modules since that's where the majority of our investment as well as the community has been going. Additionally, we've been building some tooling for ourselves to do that which you can leverage.

@kripod
Copy link

@kripod kripod commented May 26, 2020

Great addition, thank you for the awesome work!

Is it safe to always set assumeChangesOnlyAffectDirectDependencies to true if a package is marked free of sideEffects through package.json? I became curious about this while building a modern library bundler tool.

@DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented May 26, 2020

@kripod they're not necessarily related - you can have top-level statements that cause side-effects, but don't add or remove any global declarations.

This feature is really about enabling faster iteration time by only recompiling the files that directly import a file. It relies on an assumption that given A <- B <- C, a change to A won't impact C (which is not guaranteed to be true).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.