Conversation
|
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
|
Hey @sindresorhus, it's been a while. |
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
| Head extends UnknownArray = [], | ||
| Tail extends UnknownArray = [], | ||
| > = | ||
| keyof Array_ & `${number}` extends never // Is `Array_` leading a rest element or empty |
There was a problem hiding this comment.
Would be useful with a short code comment that describes the algorithm.
test-d/reverse.ts
Outdated
| // Edge cases | ||
| expectType<Reverse<[]>>([]); | ||
| expectType<Reverse<any>>(any); | ||
| expectType<Reverse<never>>(never); |
There was a problem hiding this comment.
| expectType<Reverse<never>>(never); | |
| expectType<Reverse<never>>(never); | |
| expectType<Reverse<[unknown]>>([{} as unknown] as const); |
|
#1166 (comment) was merged. You mentioned you needed it for this. |
test-d/reverse.ts
Outdated
| // Optional/undefined | ||
| expectType<Reverse<[1?, 2?, 3?]>>([3, 2, 1] as const); | ||
| expectType<Reverse<[1?, 2?, 3?], {preserveOptionalModifier: true}>>({} as [3?, 2?, 1?]); | ||
| expectType<Reverse<[1, 2?, 3?], {preserveOptionalModifier: true}>>({} as [3 | undefined, 2 | undefined, 1]); |
There was a problem hiding this comment.
Behaviour with optional elements is not correct, you can't return [3 | undefined, 2 | undefined, 1] for Reverse<[1, 2?, 3?]> because at runtime input could be [1, 2] and reversing it would produce [2, 1] which wouldn't conform to [3 | undefined, 2 | undefined, 1].
So, the output for cases containing optional elements needs fixing.
…scart`ReverseOptions` and update JsDoc
I’ve managed to:
If there are any missing tests or additional scenarios worth covering, feel free to list or add them. |
@benzaria Please check #1266 (comment). |
@som-sm I took a note of that and I fixed the problem. |
source/reverse.d.ts
Outdated
|
|
||
| @category Array | ||
| */ | ||
| export type Reverse<Array_ extends UnknownArray> = |
There was a problem hiding this comment.
Better to name the type ArrayReverse as mentioned in #1266.
I've kept the name as
ArrayReverseinstead of justReverseto match with our existingArray*types (likeArraySlice,ArraySplice).
test-d/reverse.ts
Outdated
There was a problem hiding this comment.
Cases like labelled tuples, non-tuple arrays seem missing. Can you once cross-check from the tests in #1266.
| | TailAcc // Union all possible cases when optional elements exist. | ||
| | _ArrayReverse<Rest, HeadAcc, [ | ||
| If<IsExactOptionalPropertyTypesEnabled, First, First | undefined>, // Add `| undefined` for optional elements, if `exactOptionalPropertyTypes` is disabled. | ||
| ...TailAcc, | ||
| ]> |
There was a problem hiding this comment.
This breaks tail recursion, the existing long tuple test cases don't catch this because they only contain required elements. If you instantiate with Partial<TupleOf<50, number>>, you'll see that the type breaks with a recursion depth error.
Refer #1266 for a tail recursive implementation. You can use the following test case for verification:
expectType<TupleOf<IntRange<0, 51>, number>>({} as ArrayReverse<Partial<TupleOf<50, number>>>);
ArrayReverse- Reverses the order of elements in a tuple or array type..