ts2.1: handle boolean types correctly #314
Conversation
Booleans are represented as a union of true|false, but they also have the boolean flag set so our existing code that handled unions wasn't firing. If we just treat them as unions, everything works, including more complex cases (like boolean|number => true|false|number, without the boolean flag set). More work on #295. With this change, the remaining failures in tests are mostly whitespace.
| // Note also that in a more complex union, e.g. boolean|number, then | ||
| // it's a union of three things (true|false|number) and | ||
| // ts.TypeFlags.Boolean doesn't show up at all. | ||
| if (type.flags & ts.TypeFlags.Union) { |
There was a problem hiding this comment.
I am confused here. You say that for booleans Union and Boolean flags are on. Since more than one flag is one we are in the 'default' case. Then we are in this case too, and we output 'true | false', but don't we want 'boolean' for booleans?
There was a problem hiding this comment.
The union is true|false in TS, which becomes boolean|boolean in Closure, which we dedup to boolean.
The thing I was trying to point out in this comment is that by handling the union it works everywhere, so we don't need to special-case boolean.
E.g. if the input is boolean|number, then it becomes true|false|number, which we convert to Closure boolean|boolean|number and then dedup to boolean|number.
Do you have advice on how to make the comment better?
There was a problem hiding this comment.
That makes sense, I was expecting special casing boolean. My reasoning is that if closure ever adds 'true' and 'false', this code will emit 'true|false' for input 'boolean'.
But even with special case boolean, closure adding 'true' would still break on 'boolean | number' because it will emit 'true | false | number'. So I am not going to worry about it.
Booleans are represented as a union of true|false, but they also have the boolean flag set so our existing code that handled unions wasn't firing. If we just treat them as unions, everything works, including more complex cases (like boolean|number => true|false|number, without the boolean flag set). More work on #295. With this change, the remaining failures in tests are mostly whitespace.
Booleans are represented as a union of true|false, but they
also have the boolean flag set so our existing code that handled
unions wasn't firing.
If we just treat them as unions, everything works, including more
complex cases (like boolean|number => true|false|number, without
the boolean flag set).
More work on #295. With this change, the remaining failures in
tests are mostly whitespace.