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

Declaration emit redundantly inlines optional union parameters #42165

Closed
robpalme opened this issue Dec 31, 2020 · 6 comments
Closed

Declaration emit redundantly inlines optional union parameters #42165

robpalme opened this issue Dec 31, 2020 · 6 comments
Assignees
Labels
Domain: Declaration Emit The issue relates to the emission of d.ts files Fix Available A PR has been opened for this issue In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Milestone

Comments

@robpalme
Copy link

Bug Report

🔎 Search Terms

inlining duplication declaration emit optional union parameters

🕗 Version & Regression Information

  • This is not a regression.
  • This is a stylistic issue in emitted DTS. The code is correct.
  • This is the behavior in every version I tried (3.3 ... nightly)

⏯ Playground Link

Playground link with relevant code

💻 Code

type u = 1 | 2 | 3 | 4 | 5; 

export const o = {
    mandatory(arg: u): void {arg},  // DTS references union (good)
    optional(arg?: u): void {arg},  // DTS inlines union (bad)
}

🙁 Actual behavior

DTS emit expands the definition of the union for the optional parameter.

declare type u = 1 | 2 | 3 | 4 | 5;
export declare const o: {
    mandatory(arg: u): void;
    optional(arg?: 1 | 2 | 3 | 4 | 5 | undefined): void;
};

🙂 Expected behavior

DTS emit should treat it the same as a mandatory parameter and reference it by name.

declare type u = 1 | 2 | 3 | 4 | 5;
export declare const o: {
    mandatory(arg: u): void;
    optional(arg?: u): void;
};

Related Issues

This is a very specific instance of the issue of declarations inlining types. I'm raising it because it looks like a pure win to simplify emitted DTS files. Fixing this will make other userland tricks to simplify types more effective.

If fixing this is contentious, or requires a large overhaul, please feel free to immediately close. I will not be offend because I realize there is a long tail of issues in this space and do not wish to create noise.

@andrewbranch andrewbranch added Domain: Declaration Emit The issue relates to the emission of d.ts files In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Dec 31, 2020
@andrewbranch
Copy link
Member

I bet @weswigham will know off the top of his head how feasible this is.

@weswigham
Copy link
Member

weswigham commented Dec 31, 2020

Specifically for optional property serialization, it should be feasible to see if the undefined-stripped union has an alias symbol, and to use that if available.

@OliverJAsh
Copy link
Contributor

OliverJAsh commented Jan 1, 2021

Related (it's not just unions which include undefined): #26106 (comment)

@weswigham
Copy link
Member

Yeah, the undefined case is just very common and easy to scan for. To generally fix it, we'd have to generate all possible groups of sub-unions for a union (including those with duplicates shared between sets), and see if any combinations of subsets have "better" printback (where "better" likely means shorter), and then choose the best of the available options - and even that wouldn't capture the aliases originally in unions which subtype reduced when combined (as those subtypes, and the unions they were originally in, are likely irrecoverable). So the general case isn't really easy to solve for.

@OliverJAsh
Copy link
Contributor

Will both of these issues be fixed by #42149?

@robpalme
Copy link
Author

Closing because the original code example is now fixed in TypeScript 4.2 by #42149

There's still a redundant | undefined in the emit, but I think that can be considered a different issue.

Thanks for acting on this @andrewbranch (even if you did get Sherlocked by Anders)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: Declaration Emit The issue relates to the emission of d.ts files Fix Available A PR has been opened for this issue In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants