New definition for omit that should ensure the name Omit is preserved… #37608
Conversation
… in declaration output
There was a problem hiding this comment.
I don't think this will help people unless we have a general way to not print default arguments that weren't provided (or are equal to their parameter's default). Even then it will be confusing to have a non-functional third parameter showing up in quick info.
This reminds me of being confused by basic_string's "extra" type parameters in C++ as a kid. I don't want to foist that on future generations.
| @@ -2,18 +2,18 @@ | |||
| // Repro from #29067 | |||
|
|
|||
| function test<T extends { a: string }>(obj: T) { | |||
| >test : <T extends { a: string; }>(obj: T) => Pick<T, Exclude<keyof T, "a">> & { b: string; } | |||
| >test : <T extends { a: string; }>(obj: T) => Omit<T, "a", Exclude<keyof T, "a">> & { b: string; } | |||
There was a problem hiding this comment.
I think the self-generating third argument is at least as bad as converting Omit to its definition.
There was a problem hiding this comment.
Do you think it's worth investing in hiding that argument when it matches the default? (As it always should)
There was a problem hiding this comment.
Well, it solves one problem -- reading the type -- but not the second problem: writing the type.
And if it applies everywhere, the mismatch might just be confusing. Actually, hiding any optional item entirely is likely to be confusing.
There was a problem hiding this comment.
I think I’d prefer #31205 (comment):
type Omit<T, K extends keyof any> = T extends unknown ? Pick<T, Exclude<keyof T, K>> : never;
|
I already tried to make Does this new implementation avoid that issue? |
|
@DanielRosenwasser this version preserves modifiers, unlike yours, as the mapped type is homomorphic; the 3rd type parameter is used to ensure such. |
|
I'm generally not a fan of using type parameter defaults as an ad-hoc mechanism to introduce in-situ type aliases as it clutters the declaration output and quickinfo. I still think we should introduce some mechanism for in-situ type aliases to address this, similar to a type Omit<T, K extends keyof any> =
type Keys = Exclude<keyof T, K> in
{
[P in Keys]: T[P];
}; |
sandersn
added
the
For Milestone Bug
|
@weswigham do you want to discuss this more, or look for a variant solution? If not, I'd like to close this PR to keep the number of open PRs manageable. |
|
Did we wanna discuss our preferred approach here? Extra syntax for local type aliases and better default type parameter elision could both be ways forward here. |
|
@weswigham should we keep looking for fixes for this PR or close it? |
Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>
|
@DanielRosenwasser @rbuckton @sandersn @ahejlsberg I've changed this to use the template form to preserve the alias, as suggested by @ExE-Boss. I had to make the tiniest change to templated mapped type comparisons to get the tests to pass, however. |
|
@weswigham |
|
I could swear c2f4d8a should have fixed that when I ran it locally... |
|
Oh wait, that's a different test that I just didn't notice was failing locally. Heh. Hold on. |
|
Fixed. Notable: Our linter breaks because We might need to update eslint? Or wait until the RC is out with the template mapped types to get parsing support for them in the linter? I'm unsure what, exactly, needs to go out before it'll lint without error. |
|
@typescript-bot pack this |
|
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 636005e. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the parallelized community code test suite on this PR at 636005e. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 636005e. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at 636005e. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at 636005e. You can monitor the build here. |
|
@DanielRosenwasser Here they are:Comparison Report - master..37608
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
This should also have “Fixes #31104” in the PR description. |
|
I'm gonna say this is superseded by #42524 now~ |
… in declaration output
Fixes #34793
This definition is backwards compatible (thanks to the default), but the declaration output may not be backwards compatible with the lib from older versions of TS (since we'll emit a reference to
Omitwith 3 type arguments). We could try to clean that up (maybe do some work to elide trailing type arguments if they're equivalent to the defaults), or take this as-is.