Computed names in declarations files are resolved even when non-literal, preserve computed names when expressions are entity names #60052
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot test it |
|
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
|
Hey @weswigham, the results of running the DT tests are ready. Everything looks the same! |
|
@weswigham Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
|
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@weswigham Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
|
Amazing! Has the team already discussed the intent to ship this feature or is this only an experiment? Asking because I'm wondering if https://github.com/oxc-project/oxc/tree/main/crates/oxc_isolated_declarations could bypass the need to implement something like #58771 (oxc-project/oxc#4016) and implement the behavior in this PR directly. |
|
You can't ship this before we do because it produces declaration files that can't be correctly read by current versions of TS 😉 |
…re entity name expressions
weswigham
force-pushed
the
computed-names-in-declarations-files-are-resolved
branch
from
7fec56c to
22c5c89
Compare
weswigham
force-pushed
the
computed-names-in-declarations-files-are-resolved
branch
from
f1fcd56 to
587f9e0
Compare
|
@typescript-bot test it |
|
Hey @weswigham, the results of running the DT tests are ready. Everything looks the same! |
|
@weswigham Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
|
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot test it |
|
@weswigham Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
|
Hey @weswigham, the results of running the DT tests are ready. Everything looks the same! |
|
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@weswigham Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
1 similar comment
|
@weswigham Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
|
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot test it |
|
@weswigham Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
|
Hey @weswigham, the results of running the DT tests are ready. Everything looks the same! |
|
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@weswigham Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
|
Computed properties that have a computed name of a literal type seem to be duplicated when another computed property that is not a literal type is present: // index.ts
declare const x: string;
declare const y: "y";
export class Test {
[x] = 10;
[y] = 10;
}
// index.d.ts
declare const x: string;
declare const y: "y";
export declare class Test {
[x]: number;
[y]: number;
[y]: number; // ❌
}
export {}; |
|
Does this definitely fix #60818? |
|
Not intentionally, since it's only @typescript-bot pack this |
|
Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
I think this does what it should, but what's our strategy for dealing with the fact that this PR will cause index expressions to be emitted in such a way that TS<5.8 will be unable to read them? Is this break acceptable? I think it has to be, because we really want to do it, but it seems like we're going to have to be really clear in the blog post to people that the types may have to be downleveled somehow... Concretely, before: declare class Base1 {
[x: string]: () => void;
}After: declare let prop: string;
declare class Base1 {
[prop]: () => void;
} |
Yeah. Wouldn't be the first time. It's also notable that while you do have to downlevel the declaration to inline the computed name's type to remove the |
|
What's the minimal input file that would cause a freshly-generated .d.ts file to not be readable by 5.7? |
|
In testing this, I think I found a bug: let prop!: string;
export class Base1 {
[x: string]: () => void;
[prop]!: () => void;
}
export class Base2 {
[prop]!: () => void;
[x: string]: () => void;
}The d.ts emit for this after the PR is: export declare class Base1 {
[x: string]: () => void;
}
export declare class Base2 {
[x: string]: () => void;
}But, we can't know without type analysis that the computed property is safe to drop, and the emitter can't make that decision based on whether or not isolatedDeclarations is enabled (and those kinds of emitters would preserve both). |
let prop!: string;
export class Base1 {
[prop]!: () => void;
}which will produce declare prop: string;
declare export class Base1 {
[prop]: () => void;
}except it is readable in 5.7, where I introduced the new computed name index signature behavior for classes (but not earlier), it just has an error in the
A full ID emitter will be forced to retain all possibly-conflicting names, yeah. Which is fine, duplicate declarations basically merge. Where you can, removing dupes from the output is our expected behavior, though. |
RyanCavanaugh
approved these changes
Jan 24, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR basically removes the grammar error requiring computed names in declaration files/other contexts resolve to literal types, and allows them to resolve to whatever so long as they're an entity name expression (
a.b.c), and endeavors in declaration emit to preserve those input computed names that imply index signatures wherever possible. This clears the way forisolatedDeclarationsto allow always emitting a computed property name on the output for a computed property name on the input, so long as the expression is an entity name expression.