During emit, if shape signature for the file is same as version, then update it with emitted d.ts file #48616

sheetalkamat · 2022-04-08T22:39:50Z

This change updates the signature if while computing affected file set we set it as version of the file (Whole text of the file instead of d.ts emit for perf imprrovement) using d.ts emit if it was done without using custom transformers. This will help with later updates since the shapes are initialized and smaller set of files will be updated..

sheetalkamat · 2022-04-08T22:46:37Z

src/compiler/builderPublic.ts

+         * Store the list of files that update signature during the emit
+         */
+        /*@internal*/
+        storeFilesChangingSignatureDuringEmit?: boolean;


This one is for test purposes and done so we can print it in the baseline

sheetalkamat · 2022-04-08T22:48:40Z

...ationEmit/when-declaration-file-is-referenced-through-triple-slash-but-uses-no-references.js

@@ -176,9 +176,6 @@ exports.getVar = getVar;
      ]
    },
    "exportedModulesMap": {
-      "../src/common/nominal.ts": [


This shows how we have updated the signature to d.ts emit so any update to type.d.ts wont unnecessarily update files importing nominal.ts

I don’t quite understand the expected behavior here or what’s being shown. nominal.ts exports a type that uses a global type declared in types.d.ts. A change to types.d.ts could cause a change to any file importing nominal.ts.

This one is reverting the state as if the d.ts signature was computed for the nominal.ts : That is as if buildinfo was generated before #43314
Having said that exportedModulesMap is a list of files that are imported/referenced in .d.ts so any change to those files changes their d.ts as well indirectly.
So looking at this, it does look like type.d.ts is needed in nominal.ts so i will check whats wrong.. But whatever it is, it has been that way from start.. (#39645) adds this test and its same.

sheetalkamat · 2022-04-08T22:51:22Z

tests/baselines/reference/tsc/incremental/noEmit-changes-composite.js

@@ -508,13 +508,11 @@ exitCode:: ExitStatus.DiagnosticsPresent_OutputsGenerated
 //// [/src/project/src/class.d.ts] file written with same contents
 //// [/src/project/src/class.js] file written with same contents
 //// [/src/project/src/directUse.d.ts] file written with same contents
-//// [/src/project/src/directUse.js] file written with same contents


Example of change where files aren't unnecessarily emitted

… update the signature when doing actual emit

amcasey · 2022-04-13T17:22:15Z

This change updates the signature if while computing affected file set we set it as version of the file (Whole text of the file instead of d.ts emit for perf imprrovement) using d.ts emit if it was done without using custom transformers. This will help with later updates since the shapes are initialized and smaller set of files will be updated..

I think this is what you're saying - please correct me if I've misunderstood:

We store a signature for each file we need to build so we can determine whether it needs to be rebuilt. Originally, that signature was based on the emitted .d.ts file, regardless of whether or not we had a reason to emit the .d.ts file otherwise. In order to avoid performing .d.ts emit just for signature computation, we started using a signature based on the input text when no .d.ts file was available. That was the state prior to this PR. This PR says that, if we did that and if there were no custom transformers (why does this matter?) then we can just swap out the signature and use the (better) .d.ts signature going forward.

Is that right?

sheetalkamat · 2022-04-14T17:11:50Z

In order to avoid performing .d.ts emit just for signature computation, we started using a signature based on the input text when no .d.ts file was available

Thats not what happens.. we have few optimizations when we would be computing huge set of d.ts files (like initial build) or when we are doing d.ts computation as the signature might change because it imports something with change in its d.ts indirectly. These were perf optimizations to speed up the process... Result of this is that when the file changes again, that is when we compute the d.ts for that file as well as any file that imports it to see if things have changed. Answer to that will be always yes since in first round we had sourceText and now the d.ts.. With this change, when we are emitting files and we detect that it is emitting declaration file and we are using sourceText as signature we update the signature with d.ts emit so that in next round we can correctly do the computation. This helps with not emitting some files unnecessarily as we are now have d.ts as signature and the d.ts computation from the change will match..

no custom transformers (why does this matter?)

Because custom transformers can change the generated output and we don't want and use that when we generate d.ts during signature computation

andrewbranch

Some general questions to try to help me understand the builder

andrewbranch · 2022-04-14T22:34:29Z

src/compiler/builder.ts

@@ -1098,6 +1102,33 @@ namespace ts {
            );
        }

+        function getWriteFileUpdatingSignatureCallback(writeFile: WriteFileCallback | undefined): WriteFileCallback {
+            return (fileName, text, writeByteOrderMark, onError, sourceFiles, data) => {
+                if (sourceFiles?.length === 1 && isDeclarationFileName(fileName)) {


When is sourceFiles.length === 1 true? When that’s true, does it also imply fileName === sourceFiles[0].fileName?

also imply fileName === sourceFiles[0].fileName?

No thats not true. The fileName = "d.ts" for sourceFile[0]
Its going to be single source file because its not --out options

Let me see if I can try to parse out what you’re saying:

This callback writes one output file. sourceFiles is the set of input files that correspond to that one output file. This is always 1:1 (e.g. foo.ts when writing foo.d.ts) unless outFile is set, in which case all input files get emitted together into a single .js/.d.ts.

sourceFiles?.length === 1 && isDeclarationFileName(fileName) ensures we have a 1:1 input:output and that we have a reference to the input file for the .d.ts file being written. If there were no sourceFiles or if fileName was a JS file, we obviously wouldn’t be able to do the logic in the condition. If there were many source files, it would indicate that the signature for each file is all combined into one .d.ts and so it wouldn’t make sense to use that as an up-to-date check for any of them (an change in any file’s signature would affect all files).

Is that right?

andrewbranch · 2022-04-14T22:42:28Z

...ationEmit/when-declaration-file-is-referenced-through-triple-slash-but-uses-no-references.js

@@ -176,9 +176,6 @@ exports.getVar = getVar;
      ]
    },
    "exportedModulesMap": {
-      "../src/common/nominal.ts": [


I don’t quite understand the expected behavior here or what’s being shown. nominal.ts exports a type that uses a global type declared in types.d.ts. A change to types.d.ts could cause a change to any file importing nominal.ts.

sheetalkamat · 2022-04-15T19:58:36Z

...Watch/emitAndErrorUpdates/incremental/defaultAndD/deepImportChanges/errors-for-.ts-change.js

@@ -190,9 +190,6 @@ export {};
      ]
    },
    "exportedModulesMap": {
-      "./a.ts": [


This is better example of change...
This is the test case that shows changes to transitive imports..

sheetalkamat · 2022-04-15T19:58:58Z

I don’t quite understand the expected behavior here or what’s being shown. nominal.ts exports a type that uses a global type declared in types.d.ts. A change to types.d.ts could cause a change to any file importing nominal.t

This was incorrect example and i am confused as well and will need to investigate. But the idea is that if .d.ts is not used as signature, sourceFile text and all the imports are used as if they are needed for determining d.ts impact.
To make it simple here:

/// moduleA.ts
import {} from "shared";
/// app.ts
import {} from "moduleA"

The theory is that if import of moduleA is needed when you emit app.d.ts for app.ts then change to share.ts such that share.d.ts is changed but doesnt change moduleA.d.ts can impact types in app as well app.d.ts

With #43314 we started using file text as signature in some cases for performance. And in that case we assume that all imports in the file are needed in .d.ts emit. So in that case any change to share.ts may unnecessarily update app.ts if it doesnt need moduleA import in its d.ts.

This change kind of avoids those unnecessary updates by updating the signature to d.ts emit during emit if we detect so.
https://github.com/microsoft/TypeScript/pull/48616/files#diff-65a637de3855967769ea5a0ed013a8b4cad6afccf07d094864172e80626b73d2L192 is better example showcasing this.

sheetalkamat · 2022-04-20T17:28:14Z

Ping @andrewbranch @amcasey can you please review this again so i can get this in.
Since #48616 (comment) was old behaviour and will pop up on next change anyway, i dont think its blocking this PR. Creating a investigate issue for myself to track this.
Thanks..

andrewbranch · 2022-04-20T19:09:16Z

src/compiler/builder.ts

+                        if (newSignature !== file.version) { // Update it
+                            if (host.storeFilesChangingSignatureDuringEmit) (state.filesChangingSignature ||= new Set()).add(file.resolvedPath);
+                            if (state.exportedModulesMap) BuilderState.updateExportedModules(file, file.exportedModulesFromDeclarationEmit, state.currentAffectedFilesExportedModulesMap ||= BuilderState.createManyToManyPathMap());
+                            if (state.affectedFiles && state.affectedFilesIndex! < state.affectedFiles.length) {


What is this condition? What’s the difference between storing the new signature in state.currentAffectedFilesSignatures and storing it in info.signature?

when we are emitting for affectedFiles things are maintained in currentAffectedFilesSignatures which are commited only after handling of all the affected files is completed.. Not that if we arent handling affected files that means we are doing files that are pending emit only.. So we can directly comit the change to info.signature
This is done so that affected files are handled correctly if the processing for them is cancelled midway, we can revert back as if we didnt process the change at all..
As part of #48784 i am looking into simplifying that esp now that we have affected files pending emit etc.. (this signature cache dates back when we didnt have all these things to track)

… update it with emitted d.ts file (microsoft#48616) * If we are writing dts file and have used file text as version, we can update the signature when doing actual emit * Make WriteFileCallback Api ready for future * Assert that there is only single source file when emitting d.ts file

sheetalkamat marked this pull request as ready for review April 8, 2022 22:43

typescript-bot assigned sheetalkamat Apr 8, 2022

typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Apr 8, 2022

sheetalkamat commented Apr 8, 2022

View reviewed changes

sheetalkamat requested review from amcasey and andrewbranch April 11, 2022 19:00

sheetalkamat force-pushed the dtsSignature branch from fb20c38 to e0039c8 Compare April 11, 2022 20:10

If we are writing dts file and have used file text as version, we can…

3333796

… update the signature when doing actual emit

sheetalkamat force-pushed the dtsSignature branch from e0039c8 to 3333796 Compare April 11, 2022 20:11

sheetalkamat added 2 commits April 14, 2022 11:18

Merge branch 'main' into dtsSignature

0f8c080

Make WriteFileCallback Api ready for future

33eb739

andrewbranch reviewed Apr 14, 2022

View reviewed changes

sheetalkamat added 2 commits April 15, 2022 11:46

Merge branch 'main' into dtsSignature

5359f1b

Assert that there is only single source file when emitting d.ts file

2c93709

sheetalkamat commented Apr 15, 2022

View reviewed changes

sheetalkamat mentioned this pull request Apr 20, 2022

Investigate d.ts exports map #48785

Closed

andrewbranch reviewed Apr 20, 2022

View reviewed changes

sheetalkamat mentioned this pull request Apr 20, 2022

Build improvements #48784

Merged

andrewbranch approved these changes Apr 21, 2022

View reviewed changes

Merge branch 'main' into dtsSignature

f654b5b

sheetalkamat merged commit 7a59e45 into main Apr 21, 2022

sheetalkamat deleted the dtsSignature branch April 21, 2022 18:00

sheetalkamat mentioned this pull request Jun 9, 2022

[Experiment] Cache Dts Emit result / transform #49156

Closed

sheetalkamat mentioned this pull request Nov 9, 2022

Initial build mode build emits twice #43995

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

During emit, if shape signature for the file is same as version, then update it with emitted d.ts file #48616

During emit, if shape signature for the file is same as version, then update it with emitted d.ts file #48616

sheetalkamat commented Apr 8, 2022 •

edited

Loading

sheetalkamat Apr 8, 2022

sheetalkamat Apr 8, 2022

andrewbranch Apr 14, 2022

sheetalkamat Apr 15, 2022

sheetalkamat Apr 8, 2022

amcasey commented Apr 13, 2022

sheetalkamat commented Apr 14, 2022

andrewbranch left a comment

andrewbranch Apr 14, 2022

sheetalkamat Apr 15, 2022

andrewbranch Apr 20, 2022

sheetalkamat Apr 21, 2022

andrewbranch Apr 14, 2022

sheetalkamat Apr 15, 2022

sheetalkamat commented Apr 15, 2022

sheetalkamat commented Apr 20, 2022 •

edited

Loading

andrewbranch Apr 20, 2022

sheetalkamat Apr 20, 2022 •

edited

Loading

During emit, if shape signature for the file is same as version, then update it with emitted d.ts file #48616

During emit, if shape signature for the file is same as version, then update it with emitted d.ts file #48616

Conversation

sheetalkamat commented Apr 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey commented Apr 13, 2022

sheetalkamat commented Apr 14, 2022

andrewbranch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sheetalkamat commented Apr 15, 2022

sheetalkamat commented Apr 20, 2022 • edited Loading

Choose a reason for hiding this comment

sheetalkamat Apr 20, 2022 • edited Loading

Choose a reason for hiding this comment

sheetalkamat commented Apr 8, 2022 •

edited

Loading

sheetalkamat commented Apr 20, 2022 •

edited

Loading

sheetalkamat Apr 20, 2022 •

edited

Loading