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

Extract types from object literals instead of using the type checker. #60540

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dragomirtitian
Copy link
Contributor

This PR brings TS emit closer to what an external tool could emit without type information by extracting object type directly from their source object literal where possible.

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Nov 19, 2024
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@jakebailey jakebailey requested a review from weswigham November 25, 2024 19:06
Comment on lines -42 to +44
x: string;
get x(): string;
/** my awesome setter (second in source order) */
set x(a: string);
Copy link
Member

Choose a reason for hiding this comment

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

This is a notable behavior change.

I personally believe that we should stop synthesizing props when getters were written, but this also makes me wonder what's going on below with obj4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If only one accessor is defined, this code does still use the previous behavior of creating a property signature.

@@ -28,7 +28,7 @@ var obj2 = (_c = {}, _c[hundredNum] = "bar", _c);

//// [declarationEmitPropertyNumericStringKey.d.ts]
declare const STATUS: {
readonly "404": "not found";
readonly ["404"]: "not found";
Copy link
Member

Choose a reason for hiding this comment

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

This is a change, but is probably fine.

@@ -52,7 +52,7 @@ export declare enum MouseButton {
NO_BUTTON = 0
}
export declare const DOMMouseButton: {
'-1': MouseButton;
"-1": MouseButton;
Copy link
Member

Choose a reason for hiding this comment

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

The quote style in the original code is single; if we're copying this, shouldn't the quote have stayed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit of a strange one, because sometimes TS normalizes strings that come from values and sometimes it does not. If you assign a string to a const, TS never preserved those as written and replaced the string delimiters. I preserved this behavior, and it is now applied to string property names:

const x = '1' // We get "1" in d.ts
const y = { '1': ''} // We get '1' in d.ts.

Copy link
Member

Choose a reason for hiding this comment

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

I guess I don't know why this is a desirable change, if the behavior differs depending on where the string is used...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened a PR that shows what the impact of preserving delimiters more widely would be: #60729

@@ -16,6 +16,6 @@ var m1;
//// [stringLiteralObjectLiteralDeclaration1.d.ts]
declare namespace m1 {
var n: {
'foo bar': number;
"foo bar": number;
Copy link
Member

Choose a reason for hiding this comment

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

Same quoting question here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Status: Not started
Development

Successfully merging this pull request may close these issues.

3 participants