Process a union or intersection type of all strings as a string - #675
Merged
Perryvw merged 1 commit intoJul 22, 2019
Merged
Conversation
Perryvw
reviewed
Jul 20, 2019
ark120202
reviewed
Jul 20, 2019
| expect(result).toBe(inp.toUpperCase()); | ||
| }); | ||
|
|
||
| test.each(["hello", "test", "hello test"])("string.toUpperCase (union) (%p)", inp => { |
Contributor
There was a problem hiding this comment.
- This test isn't really related to string.toUpperCase, but to general array string detection, so it should be be placed before
string constrained generic foreach (%p)and be renamed - There's no need for test.each, since it's not the main reason for this test
- The code inside template is indented with 2 spaces instead of 4
- Variables shouldn't begin with underscore, typescript uses it for it's unused diagnostics
- I don't see much reason to over-complicate test code with inference logic, something like this would be enough to prove test's point:
const union: "foo" | "bar" = "foo";
return union.length;
Contributor
Author
There was a problem hiding this comment.
- I was mainly concerned with
string.*being transpiled correctly, hence I thought it made sense to have it around here. I'll move it :) - I guess you are right.
- Woopsie 👍
- Gotcha - fixed in the next push.
- Your proposal doesn't cover this.
unionhere is just a string literal which already supports.length. Enforcing union to be a string type triggers the type guard which appears to compare the string to a union of each or-condition. Inside the if-block,unionis now inferred as an actual union of strings.^ does the trick, though!const union: string = "foo"; if (union === "foo" || union === "bar") { return union.length; } return 0;
Contributor
There was a problem hiding this comment.
Oh, looks like TS ignores type annotation in this case and only something like const union = "foo" as "foo" | "bar"; works
martinjlowm
force-pushed
the
enhancement/detect-string-from-union
branch
from
July 21, 2019 11:37
9ca89e5 to
9da0d68
Compare
Contributor
Author
|
I've just pushed new changes based off of your feedback - thanks :) Perhaps the test could have a better description, I'm open to suggestions. |
Perryvw
approved these changes
Jul 21, 2019
ark120202
reviewed
Jul 21, 2019
| } | ||
| ); | ||
|
|
||
| test.each(["foo"])("scoped string-union inference (%p)", inp => { |
Contributor
There was a problem hiding this comment.
.each with a single case can be removed completely and be inlined to code.
martinjlowm
force-pushed
the
enhancement/detect-string-from-union
branch
from
July 21, 2019 19:48
9da0d68 to
3b284fe
Compare
ark120202
approved these changes
Jul 21, 2019
This file contains hidden or 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
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.
Prior to this PR, the following snippet would fail to transpile
toUpperCasetoupper.Now, all subtypes of a union is checked and the following is transpiled from:
to