fix(v2): Improve `take` typings for tuples #649

Brodzko · 2024-04-20T14:40:42Z

Based on #647 (review), porting that MR into v2 and improved typing tests.

Please do let me know if there is anything else needed to merging into beta - I noticed pre-commit hooks failing due to a bunch of TS errors that are unrelated to my changes, so I assume that is expected for now?

Make sure that you:

Typedoc added for new methods and updated for changed
Tests added for new methods and updated for changed
New methods added to src/index.ts
New methods added to mapping.md

We use semantic PR titles to automate the release process!

https://conventionalcommits.org

PRs should be titled following using the format: < TYPE >(< scope >)?: description

Available Types:

feat: new functions, and changes to a function's type that would impact users.
fix: changes to the runtime behavior of an existing function, or refinements to it's type that shouldn't impact most users.
perf: changes to function implementations that improve a functions runtime performance.
refactor: changes to function implementations that are neither fix nor perf
test: tests-only changes (transparent to users of the function).
docs: changes to the documentation of a function or the documentation site.
build, ci, style, chore, and revert: are only relevant for the internals of the library.

For scope put the name of the function you are working on (either new or
existing).

codesandbox-ci · 2024-04-20T14:41:46Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 13bd512:

Sandbox	Source
remeda-example-vanilla	Configuration

codecov · 2024-04-20T14:43:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (beta@ca59256). Click here to learn what that means.

Additional details and impacted files

@@           Coverage Diff            @@
##             beta      #649   +/-   ##
========================================
  Coverage        ?   100.00%           
========================================
  Files           ?       146           
  Lines           ?     10079           
  Branches        ?       736           
========================================
  Hits            ?     10079           
  Misses          ?         0           
  Partials        ?         0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

eranhirsch · 2024-04-20T14:46:26Z

Regarding your precommit hooks, don't forget to npm ci when moving between the master branch and the beta branch, the dependencies have changed considerably and you can't use the tooling without updating them first.

eranhirsch · 2024-04-20T14:50:14Z

src/take.ts

+const takeImplementation = <T extends ReadonlyArray<unknown>, N extends number>(
+ array: T,
+ n: N,
+): unknown => _reduceLazy(array, lazyImplementation(n));


If we are changing this function with breaking changes lets also change the implementation to use Array.prototype.slice to perform the dataFirst implementation. It's funky that we use our own reducer for this.

Is this what you mean?

Also, what do you think about supporting negative range, same as slice while we're at it? I.e. R.take(arr, -2) - return last 2 elements of arr

Would the typing work?

Well, been looking at it and I'm not sure we can get them working the same as for taking from he head part. Especially in "headed" cases like [number, string, ...Array<boolean>] there really doesn't seem to be a "good" way to define the output array.

However while thinking about this I also realized the same problem exists for regular take in "tailed" cases and that my tests do not make sense in those cases either.

Take const arr = [1, 2] as [...Array<number>, string]. What should be the output type of take(arr, n) for any n? Best we could do is maybe something like Array<number> | [...Array<number>, string], or Array<number | string>, but neither seems as a good choice?

Anyhow if we solve this ^, than I think for N < 0, Take<T, N> = Reverse<Take<Reverse<T>, -N>> should do the trick?

eranhirsch

Typing take is similar to how you'd implement a hasAtMost type predicate. If the input is number[] and N=3, you'd want the output to be either [number?, number?, number?] or [] | [number] | [number, number] | [number, number, number] (preferably), and not number[].

It's not the typing is incorrect right now, it's just that it's less useful than it could be, if we are already looking into narrowing the type. The most common inputs in general would probably be regular arrays string[], fixed tuples like [string, string, string], and maybe non-empty arrays [boolean, ...boolean[]], so those are the types we should focus on providing a better experience for. We currently do the fixed tuple well, but not the other two.

Regarding handling negative N values - I think the current implementation makes the most sense, as it aligns better with what the expectation of the semantics of the function is, currently there is a correlation between the size of N and the number of items you get, so if N1 > N2 the number of elements you get back is |N1| >= |N2|, but if you support negative values, then this doesn't hold anymore: N1 = 1, N2 = -3 --> |N1| === 1, |N2| === 3.

Another issue, there are no type tests where N is a number and not a const. Assume that a lot of users use take with a non-const number. We need to make sure that our typing makes sense for them.

You can test N as a number by doing 1 as number as an operand.

eranhirsch · 2024-04-22T08:51:29Z

src/take.ts

+ ? TakeAcc<Tail, N, [...Acc, Head]>
+ : [...Acc, ...T];
+
+type Take<T extends ReadonlyArray<unknown>, N extends number> = TakeAcc<T, N>;


Why do we need this? Acc has a default value so calling TakeAcc<T, N> would be equivalent to calling Take<T, N>

Suggested change

type Take<T extends ReadonlyArray<unknown>, N extends number> = TakeAcc<T, N>;

eranhirsch · 2024-04-22T08:51:52Z

src/take.ts

 import type { LazyEvaluator } from "./pipe";
 import { purry } from "./purry";

+type TakeAcc<


Once we remove Take we can rename this type Take.

Suggested change

type TakeAcc<

type Take<

eranhirsch · 2024-04-22T08:55:24Z

src/take.ts

 import type { LazyEvaluator } from "./pipe";
 import { purry } from "./purry";

+type TakeAcc<
+ T extends ReadonlyArray<unknown>,


We usually use IterableContainer as a type, it's taken from the typescript lib and might have some slight difference in some edge-cases (it adds | [] to the type).

Suggested change

T extends ReadonlyArray<unknown>,

T extends IterableContainer,

eranhirsch · 2024-04-22T08:56:37Z

src/take.ts

+type TakeAcc<
+ T extends ReadonlyArray<unknown>,
+ N extends number,
+ Acc extends ReadonlyArray<unknown> = [],


Prefer not using acronyms and abbreviations, they make the code less readable for people who English isn't their first language.

Suggested change

Acc extends ReadonlyArray<unknown> = [],

Accumulator extends ReadonlyArray<unknown> = [],

eranhirsch · 2024-04-22T08:58:02Z

src/take.test.ts

@@ -5,6 +5,14 @@ describe("data_first", () => {
 it("take", () => {
 expect(take([1, 2, 3, 4, 3, 2, 1], 3)).toEqual([1, 2, 3]);
 });
+
+ it("returns the whole array if N is greater than length", () => {
+ expect(take([1, 2, 3] as const, 10)).toEqual([1, 2, 3]);


runtime tests don't need as const, it's meaningless to what they test.

Suggested change

expect(take([1, 2, 3] as const, 10)).toEqual([1, 2, 3]);

expect(take([1, 2, 3], 10)).toEqual([1, 2, 3]);

eranhirsch · 2024-04-22T09:24:51Z

src/take.test.ts

+ expectTypeOf(take(mix, 3)).toEqualTypeOf<
+ ["cat" | "dog", ...Array<123 | 456>]
+ >();


And similarly: ["cat" | "dog"] | ["cat" | "dog", 123 | 456] | ["cat" | "dog", 123 | 456, 123 | 456]

eranhirsch · 2024-04-22T09:26:02Z

src/take.test.ts

+ it("handles shorter heads", () => {
+ const shortHead = [1, "foo", true, false] as [
+ number,
+ string,
+ ...Array<boolean>,
+ ];
+ expectTypeOf(take(shortHead, 3)).toEqualTypeOf<
+ [number, string, ...Array<boolean>]
+ >();
+
+ const shortHeadReadonly = [1, "foo", true, false] as readonly [
+ number,
+ string,
+ ...Array<boolean>,
+ ];
+ expectTypeOf(take(shortHeadReadonly, 3)).toEqualTypeOf<
+ [number, string, ...Array<boolean>]
+ >();
+ });


Is this test redundant, it feels like it's already tested as part of the tuples with rest tails...

eranhirsch · 2024-04-22T09:26:43Z

src/take.test.ts

+ });
+
+ it("handles regular arrays", () => {
+ const input = [1, 2, 3];


cast this to an array, i'm not sure the type isn't a tuple here...

you don't need values for type tests, only types...

Suggested change

const input = [1, 2, 3];

const input = [] as Array<number>;

eranhirsch · 2024-04-22T09:27:17Z

src/take.test.ts

+
+ expectTypeOf(result).toEqualTypeOf<[1, 2]>();
+
+ const inputReadonly = [1, 2, 3, 4] as readonly [1, 2, 3, 4];


Suggested change

const inputReadonly = [1, 2, 3, 4] as readonly [1, 2, 3, 4];

const inputReadonly = [1, 2, 3, 4] as const;

eranhirsch · 2024-04-22T09:29:11Z

src/take.ts

+ ? Acc
+ : T extends readonly [infer Head, ...infer Tail]
+ ? TakeAcc<Tail, N, [...Acc, Head]>
+ : [...Acc, ...T];


I'm not sure about this branch. Can we return a narrowed type that would take N into account? See my questions in the tests.

Brodzko · 2024-04-27T22:50:45Z

Typing take is similar to how you'd implement a hasAtMost type predicate. If the input is number[] and N=3, you'd want the output to be either [number?, number?, number?] or [] | [number] | [number, number] | [number, number, number] (preferably), and not number[].

I see your point, but am not sure about the ergonomics of types like that. Consider e.g. for [...Array<number>, string] and N=3 the resulting type would have to be

type CorrectType = [string] | [number, string] | [number, number] | [number, number, string] | [number, number, number]

For large N this would quickly become unreadable and maybe even hit some performance issues when type checking?
The fact it would necessitate following calls to hasAtLeast to narrow the type further can also be seen as both good and bad I suppose.

I assume for N = number this would end up being like this?

type TypeForNumber = Array<number> | [...Array<number>, string]`

eranhirsch · 2024-04-29T18:45:02Z

It's a question of practicality. In most cases, if you are doing take(N) with a const N you are either using a small N because you want to access specific items in the array (e.g. arr[3]), or a large N where you probably don't care about the type of any of the individual elements (maybe returning a paginated result from a server). In the former case, every bit of typing information is helpful; it can reduce the need to perform redundant runtime checks or handle overly wide typing; in the latter case, you can cast your number as number and "turn off" all these advanced typing if the typing is causing issues.

Typescript performance isn't a problem until we have concrete information that it is. From my experience with hasAtLeast, typescript can handle up to about 50 iterations of recursive types before it fails.

TL;DR - my point is, for low constant N's, the better the typing, the more useful it is, and the typing can always be turned off for larger Ns if they do create an issue, so we should strive to maximize the type utility here.

Martin Brodziansky added 4 commits April 20, 2024 15:22

Add preliminary test

96c01e1

cp: Implement better types and add more tests

9cff22a

cp: Fix types for pipes

373af25

Add extra tests and change implementation to fix failing cases

764bb22

Brodzko mentioned this pull request Apr 20, 2024

fix(take): Improve take typings for tuples #647

Closed

4 tasks

Fix mistake in tests

93f4e6b

eranhirsch reviewed Apr 20, 2024

View reviewed changes

Add extra tests and change implementation to fix failed cases

13bd512

Brodzko force-pushed the v2-take-typings branch from f6346f6 to 13bd512 Compare April 20, 2024 18:08

eranhirsch added the v2 PRs that are part of the v2 release label Apr 22, 2024

eranhirsch requested changes Apr 22, 2024

View reviewed changes

eranhirsch deleted the branch remeda:beta May 31, 2024 06:34

eranhirsch closed this May 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(v2): Improve `take` typings for tuples #649

fix(v2): Improve `take` typings for tuples #649

Brodzko commented Apr 20, 2024

codesandbox-ci bot commented Apr 20, 2024 •

edited

codecov bot commented Apr 20, 2024 •

edited

eranhirsch commented Apr 20, 2024

eranhirsch Apr 20, 2024

Brodzko Apr 20, 2024

eranhirsch Apr 20, 2024

Brodzko Apr 21, 2024

Brodzko Apr 21, 2024

eranhirsch left a comment

eranhirsch Apr 22, 2024

eranhirsch Apr 22, 2024

eranhirsch Apr 22, 2024

eranhirsch Apr 22, 2024

eranhirsch Apr 22, 2024

eranhirsch Apr 22, 2024

eranhirsch Apr 22, 2024

eranhirsch Apr 22, 2024

eranhirsch Apr 22, 2024

eranhirsch Apr 22, 2024

Brodzko commented Apr 27, 2024

eranhirsch commented Apr 29, 2024

	T extends ReadonlyArray<unknown>,
	T extends IterableContainer,

	Acc extends ReadonlyArray<unknown> = [],
	Accumulator extends ReadonlyArray<unknown> = [],

	expect(take([1, 2, 3] as const, 10)).toEqual([1, 2, 3]);
	expect(take([1, 2, 3], 10)).toEqual([1, 2, 3]);


		expectTypeOf(result).toEqualTypeOf<[1, 2]>();

		const inputReadonly = [1, 2, 3, 4] as readonly [1, 2, 3, 4];

	const inputReadonly = [1, 2, 3, 4] as readonly [1, 2, 3, 4];
	const inputReadonly = [1, 2, 3, 4] as const;

fix(v2): Improve take typings for tuples #649

fix(v2): Improve take typings for tuples #649

Conversation

Brodzko commented Apr 20, 2024

Available Types:

codesandbox-ci bot commented Apr 20, 2024 • edited

codecov bot commented Apr 20, 2024 • edited

Codecov Report

eranhirsch commented Apr 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eranhirsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Brodzko commented Apr 27, 2024

eranhirsch commented Apr 29, 2024

fix(v2): Improve `take` typings for tuples #649

fix(v2): Improve `take` typings for tuples #649

codesandbox-ci bot commented Apr 20, 2024 •

edited

codecov bot commented Apr 20, 2024 •

edited