Fixed build mode false negative from export-star facade#4443
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes an incremental --build false-negative where a composite project’s emitted .d.ts is text-stable (e.g. export * from ...) while its re-exported API changes via referenced projects, causing downstream projects to not rebuild when they should.
Changes:
- Extend composite
.d.tsemit signature computation to incorporate versions of transitively re-exported declaration files. - Add a new
tsc -bregression test scenario for anexport *facade across composite project references. - Update/introduce
tsbuildreference baselines reflecting the newemitSignaturesbehavior in.tsbuildinfo.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/execute/incremental/emitfileshandler.go | Updates composite .d.ts signature tracking to account for transitive export ... from declaration dependencies. |
| internal/execute/tsctests/tscbuild_test.go | Adds a new reexportFacade incremental build regression test with edits to a referenced project’s exported type. |
| testdata/baselines/reference/tsbuild/reexportFacade/export-star-facade-through-composite-project-references.js | New baseline capturing the new regression scenario output. |
| testdata/baselines/reference/tsbuild/projectReferenceRedirect/uses-correct-project-reference-redirect-when-file-belongs-to-multiple-sub-projects.js | Baseline update showing new emitSignatures recorded in build info. |
| testdata/baselines/reference/tsbuild/moduleSpecifiers/synthesized-module-specifiers-across-projects-resolve-correctly.js | Baseline update reflecting emitSignatures inclusion for cross-project specifier scenarios. |
| testdata/baselines/reference/tsbuild/moduleResolution/resolution-from-d.ts-of-referenced-project.js | Baseline update reflecting emitSignatures inclusion for referenced-project .d.ts resolution. |
johnfav03
changed the title
| if newSignature == "" { | ||
| newSignature = h.program.snapshot.computeHash(getTextHandlingSourceMapForSignature(text, data)) | ||
| } | ||
| // A composite project's d.ts can re-export another project's API via `export ... from "..."` whose text | ||
| // is stable even when that API changes; fold the re-exported d.ts versions in so the signature reflects it. | ||
| if reexportSuffix := h.getDeclarationReexportSignatureSuffix(file); reexportSuffix != "" { | ||
| newSignature = h.program.snapshot.computeHash(newSignature + "\n" + reexportSuffix) | ||
| } |
There was a problem hiding this comment.
This double hashes, no?
Is a newline how we actually combine things elsewhere, just a newline? Hopefully this doesn't end up with collisions.
There was a problem hiding this comment.
Good callout on the double hashing. For the newline combining, diagnosticToStringBuilder uses newlines in the same way in snapshot.go.
|
|
||
| // Returns versions of declaration (.d.ts) files in the transitive `export ... from` closure of the file. | ||
| // Same-project re-exports resolve to .ts sources and are skipped | ||
| func (h *emitFilesHandler) getDeclarationReexportSignatureSuffix(file *ast.SourceFile) string { |
There was a problem hiding this comment.
Does this correspond to something in Strada I could compare to?
There was a problem hiding this comment.
Strada unfortunately has the same behavior for this issue.
|
In general this seems not unreasonable, though it does seem like a lot of text to go into the key (but it is not saved, I know). Do want a vibe check by @andrewbranch, however. |
# Conflicts: # testdata/baselines/reference/tsbuild/moduleSpecifiers/synthesized-module-specifiers-across-projects-resolve-correctly.js # testdata/baselines/reference/tsbuild/projectReferenceRedirect/uses-correct-project-reference-redirect-when-file-belongs-to-multiple-sub-projects.js
|
This should be marked for post-7.0, right? |
I think that would be appropriate - this issue also existed in 6.0, so it doesn't seem super time critical |
|
Yes, per microsoft/TypeScript#63579 (comment) I thought based on this being sent now that it was a new bug |
Fixes microsoft/TypeScript#63579