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

method.getSignature().getReturnType().getText() returns union type instead of type alias in 10.x #943

Open
AlCalzone opened this issue Mar 1, 2021 · 8 comments
Labels
bug compiler api Issue related to an issue in the TypeScript Compiler API

Comments

@AlCalzone
Copy link

AlCalzone commented Mar 1, 2021

I am using ts-morph to auto-generate documentation for a project of mine and just upgraded from ts-morph from 9.1.0 to 10.0.1.

Describe the bug

Version: 10.0.1

Prior to the upgrade, this method had an inferred return type of

Promise<{ currentValue: Maybe<boolean>; targetValue: boolean | undefined; duration: Duration | undefined; } | undefined>

whereas after the update, the return type gets printed as

Promise<{ currentValue: boolean | ("unknown" & { __brand: boolean; }); targetValue: boolean | undefined; duration: Duration | undefined; } | undefined>

where currentValue gets printed with the union type behind Maybe<boolean>.

I have verified with git bisect that the culprit is indeed not the update to TypeScript 4.2, but the ts-morph update. I didn't find anything regarding this change in the changelog, so I'm assuming it is not intended.

To Reproduce

git clone https://github.com/zwave-js/node-zwave-js
git checkout 6ffcf0f0104cbfb1c1af793b2a8fc291eb171c9d
yarn install && yarn run docs:generate
# you should see no changes here
git checkout 3373caa84e16c78999101fefbc82f4dd4b49f774
yarn install && yarn run docs:generate
# docs/api/CCs/BinarySwitch.md changes as described above

Expected behavior

No changes to docs/api/CCs/BinarySwitch.md like before the update.

@dsherret
Copy link
Owner

dsherret commented Mar 1, 2021

@AlCalzone this functionality is all from the compiler API so this was because of TS 4.2, likely the "Smarter Type Alias Preservation" change: https://devblogs.microsoft.com/typescript/announcing-typescript-4-2/#smarter-type-alias-preservation

It looks like in this case it's not doing a great job. How does it display in your editor when your editor is using TS 4.2?

@AlCalzone
Copy link
Author

AlCalzone commented Mar 1, 2021

The editor is displaying a third variant, which looks like it resolves the original type to its union constituents but doesn't go further after that:
grafik

Guess its not a bug in ts-morph then - feel free to close.

@dsherret
Copy link
Owner

dsherret commented Mar 1, 2021

That's more what I would expect (because of the || false). I wonder if this is just some type format flags thing that needs to be provided to an arg of getText(...) in order to get BrandedUnknown<boolean>.

@dsherret
Copy link
Owner

dsherret commented Mar 1, 2021

@AlCalzone Maybe try providing ts.TypeFormatFlags.UseAliasDefinedOutsideCurrentScope to the second argument of type.getText()

@AlCalzone
Copy link
Author

I can reproduce on the TS playground that this regressed in 4.2.2:
microsoft/TypeScript#43031
4.1.5 correctly preserves the alias Maybe<boolean> which has boolean as a union constituent.

No dice with UseAliasDefinedOutsideCurrentScope sadly.

@dsherret
Copy link
Owner

dsherret commented Mar 1, 2021

@AlCalzone hmmm... must be a way considering the language service shows that.

I think though that boolean | BrandedUnknown<boolean> is correct because false in || false is the boolean type without a brand. I think it was a bug before.

@AlCalzone
Copy link
Author

AlCalzone commented Mar 1, 2021

But Maybe<boolean> is boolean | ("unknown" & { __brand: boolean }), which includes boolean itself without a brand.

I could get to the type the language service prints by changing the scope for getText(), but that is still unnecessarily complicated.

@dsherret
Copy link
Owner

dsherret commented Mar 1, 2021

@AlCalzone oh, you are right! I was lazily/incorrectly only looking at the Brand type.

@dsherret dsherret added compiler api Issue related to an issue in the TypeScript Compiler API bug and removed question labels Mar 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug compiler api Issue related to an issue in the TypeScript Compiler API
Projects
None yet
Development

No branches or pull requests

2 participants