feat(every): Add `every` #631

Brodzko · 2024-04-10T21:22:44Z

Following up on conversation in #624, I started with implementing every.

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-10T21:23:10Z

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 64ca4ef:

Sandbox	Source
remeda-example-vanilla	Configuration

codecov · 2024-04-10T21:24:44Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.62%. Comparing base (96a4e82) to head (64ca4ef).
Report is 90 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #631      +/-   ##
==========================================
+ Coverage   98.41%   98.62%   +0.20%     
==========================================
  Files         127      155      +28     
  Lines        7954    11186    +3232     
  Branches      724      941     +217     
==========================================
+ Hits         7828    11032    +3204     
- Misses        123      151      +28     
  Partials        3        3

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

cjquines · 2024-04-16T14:54:00Z

mapping.md

@@ -109,6 +109,7 @@ documentation when migrating._
 | [`unique`](https://remedajs.com/docs/#unique)                       | [`uniq`](https://lodash.com/docs/4.17.15#uniq)                           | [`uniq`](https://ramdajs.com/docs/#uniq)                           |
 | [`uniqueBy`](https://remedajs.com/docs/#uniqueBy)                   | [`uniqBy`](https://lodash.com/docs/4.17.15#uniqBy)                       | [`uniqBy`](https://ramdajs.com/docs/#uniqBy)                       |
 | [`uniqueWith`](https://remedajs.com/docs/#uniqueWith)               | [`uniqWith`](https://lodash.com/docs/4.17.15#uniqWith)                   | [`uniqWith`](https://ramdajs.com/docs/#uniqWith)                   |
+| [`every`](https://remedajs.com/docs/#every)                         | [`every`](https://lodash.com/docs/4.17.15#every)                         |                                                                    |


nit: let's sort this

cjquines · 2024-04-16T15:05:45Z

src/every.ts

+ * NOTE: If you want to use this inside `conditional` predicates and preserve proper type inference,
+ * make sure to define the final type guard outside of the pipe. This is due to Typescript's limitation
+ * in generic type inference. See examples below for details.
+ *


question: what do you feel about dropping this? i think this is a problem with conditional and not every, and so notes like this should go into usage of conditional (because they also apply to other predicates)

also, i would say things like this should go in one of the conditional tests as like, a known issue

(rather than the jsdoc, which appears every time we hover over the function)

Removed 👍

cjquines · 2024-04-16T15:11:42Z

src/every.test.ts

+    expect(data).toBe(true);
+  });
+
+  test("narrows types when used with filter", () => {


nit: i wouldnt really think of this as a unit test... it tests filter more than it does every here, since we do already have a test of every narrowing properly?

cjquines · 2024-04-16T15:12:30Z

src/every.ts

+ *    R.every(['a', 0, 'b'], R.isString) // => false
+ * @dataFirst
+ * @indexed
+ * @pipeable


issue: i think we only use @pipeable for lazy functions, so we should remove this

(or at least, we should)

My first pr after releasing v2 would be to rename pipeable as lazy

cjquines · 2024-04-16T15:13:18Z

oops sorry for the late review, was busy with my Day Job

Brodzko · 2024-04-19T22:24:28Z

@cjquines no worries, same here. Pushed updates to your previous comments :)

cjquines

lgtm, will merge after todo

cjquines · 2024-04-22T14:49:10Z

mapping.md

+| [`entries`](https://remedajs.com/docs/#entries)                     |
+| [`toPairs`](https://lodash.com/docs/4.17.15#toPairs)                | [`toPairs`](https://ramdajs.com/docs/#toPairs)                           |


todo: formatting here is now incorrect

Not sure if I see what's wrong here exactly?

this should be a single line

Ah 🤦 ngl that took me a while to finally notice. Fixed, thanks! 🦅 👁️

cjquines

scratch that i actually have some more questions

cjquines · 2024-04-22T14:53:33Z

src/every.test.ts

+  });
+});
+
+describe("data-last", () => {


suggestion: rewrite these tests to use pipe

i think we mostly care about data-last usages in pipes, so i'd push for these tests to be in pipes

cjquines · 2024-04-22T14:53:54Z

src/every.test.ts

+  });
+});
+
+describe("in pipe", () => {


question: what does this test?

there's no inference being tested here

eranhirsch · 2024-04-22T15:15:47Z

src/_types.ts

+export type TypePred<T, S extends T> = (input: T) => input is S;
+
+export type TypePredIndexed<T, S extends T> = (
+  input: T,
+  index: number,
+  array: ReadonlyArray<T>,
+) => input is S;
+
+export type TypePredIndexedOptional<T, S extends T> = (
+  input: T,
+  index?: number,
+  array?: ReadonlyArray<T>,
+) => input is S;


Don't define these here, all of these types are going away in v2. They aren't useful because the hide a very basic core concept of typing

Where would be a better place to put them then, while in v1?

put the types inline

Put them inline in the file, the reasoning behind putting them in _types was that I immediately needed the same thing for some in the next PR, but let's take it step by step

@eranhirsch lgty?

eranhirsch · 2024-04-22T15:17:13Z

src/every.ts

+export namespace every {
+  export function indexed<T, S extends T>(
+    data: ReadonlyArray<T>,
+    predicate: TypePredIndexed<T, S>,
+  ): data is Array<S>;
+  export function indexed<T>(
+    data: ReadonlyArray<T>,
+    predicate: PredIndexed<T, boolean>,
+  ): boolean;
+  export function indexed<T, S extends T>(
+    predicate: TypePredIndexed<T, S>,
+  ): (data: ReadonlyArray<T>) => data is Array<S>;
+  export function indexed<T>(
+    predicate: PredIndexed<T, boolean>,
+  ): (data: ReadonlyArray<T>) => boolean;
+  export function indexed(): unknown {
+    return purry(_every(true), arguments);
+  }
+}


Also the indexed version is not needed, make the regular version support the indexed version.

eranhirsch · 2024-04-23T06:16:42Z

src/every.test.ts

+    const data = every(isNumber)(input);
+    expect(data).toBe(true);
+    if (every(isNumber)(input)) {
+      expectTypeOf(input).toEqualTypeOf<Array<number>>();


don't mix type testing with runtime testing; they run at different times in the test cycle and thus bind two unrelated concerns.

That's why I prefer having a "runtime" block and a "typing" block.

eranhirsch · 2024-04-23T06:20:07Z

src/every.ts

+ * @example
+ *    // using `R.every` as a type predicate within `conditional`
+ *    const allNumbers = R.every(R.isNumber);
+ *    const result = R.pipe(
+ *      R.conditional(
+ *        [allNumbers, val => {...}]
+ *                  // ^? number[]
+ *        ...
+ *      )
+ *    )


I don't know if we support multiple example tags in our docs. This seems like a really big example using a utility that people might not know or understand; it makes the example more complex than it should be.

Our examples aren't usually full-blown usage examples; they just show a basic, almost trivial, use case so that people understand what the params look like and how they relate to the output.

eranhirsch · 2024-04-23T06:20:54Z

src/every.ts

+  return purry(_every(false), arguments);
+}
+
+const _every =


We are dropping these underscore marker for private implementations. Use a full name for the function, everyImplementation

… case it gets resolved

Brodzko · 2024-04-27T22:41:01Z

Sorry, got caught up at work. I'll try to get to the other PRs during the weekend too.

Tried addressing most of you guys' comments and posted some questions to others :)

eranhirsch · 2024-04-30T12:19:33Z

src/every.ts

+  data: ReadonlyArray<T>,
+  predicate: PredIndexed<T, boolean>,
+): boolean =>
+  data.every((element, index, array) => predicate(element, index, array));


Check out similar functions in V2, you don't need to create the arrow function here, it's redundant for our use-case

Suggested change

data.every((element, index, array) => predicate(element, index, array));

data.every(predicate);

Did this, nut had to disable no-array-callback-reference for it to be legal :)

yes, that rule is disabled altogether in v2

eranhirsch · 2024-04-30T12:20:34Z

src/every.ts

+type TypePredIndexed<T, S extends T> = (
+  input: T,
+  index: number,
+  array: ReadonlyArray<T>,
+) => input is S;


Inline this, in v2 all functions have the function operand stated explicitly, these make it harder to read the function signature to understand what it accepts.

eranhirsch · 2024-04-30T12:21:00Z

src/every.ts

@@ -0,0 +1,63 @@
+import type { PredIndexed } from "./_types";


Similarly, these are removed in v2, don't use them

eranhirsch

Sorry for the comments so late in the development. I've been working mainly on V2 so didn't have time to check other stuff. Because the release of v2 is happening very soon and it's likely we'll merge these changes only after releasing v2 it needs to match the style for v2.

eranhirsch · 2024-05-01T06:23:09Z

src/conditional.test.ts

+      ]),
+    );
+
+    expect(resultBroken).toBe(true);


This is a runtime check in a typing test. Do we need this? it doesn't test anything that is related to the bug

eranhirsch · 2024-05-01T06:25:53Z

src/conditional.test.ts

+      ]),
+    );
+
+    expect(resultCorrect).toBe(true);


eranhirsch · 2024-05-01T06:29:31Z

src/every.test.ts

+    test("returns correct values on simple arrays", () => {
+      const trueInput = [1, 2, 3];
+      const trueData = every(trueInput, (val) => val < 5);
+      expect(trueData).toBe(true);
+
+      const falseInput = ["foo", "bar", "buzz"];
+      const falseData = every(falseInput, (val) => val.length > 3);
+      expect(falseData).toBe(false);
+    });


Keep tests simple and short; it's better to have more tests than to have tests do more; that way, when a test fails, the actual failure is reported. And we can debug the specific case instead of having to comment out non-failing parts.

Suggested change

test("returns correct values on simple arrays", () => {

const trueInput = [1, 2, 3];

const trueData = every(trueInput, (val) => val < 5);

expect(trueData).toBe(true);

const falseInput = ["foo", "bar", "buzz"];

const falseData = every(falseInput, (val) => val.length > 3);

expect(falseData).toBe(false);

});

test("all values meet the criteria", () => {

expect(every([1, 2, 3], (val) => val < 5)).toBe(true);

});

test("a value doesn't meet the criteria", () => {

expect(every(["foo", "bar", "buzz"], (val) => val.length > 3)).toBe(false);

});

eranhirsch · 2024-05-01T06:30:07Z

src/every.test.ts

+      expect(falseData).toBe(false);
+    });
+
+    // empty set check


The test name describes it, we don't need documentation inside tests

Suggested change

// empty set check

eranhirsch · 2024-05-01T06:31:27Z

src/every.test.ts

+      const input = [1, 2, 3];
+      const data = every(input, (_, index) => index < 5);
+      expect(data).toBe(true);


👍🏼 nice, this is an advantage of moving indexed predicates to the main signature, we test them like regular logic and not disconnected from other concerns!

eranhirsch · 2024-05-01T06:37:03Z

src/every.test.ts

+    const dataPipe = pipe(
+      input,
+      every((val, index, arr) => {
+        expectTypeOf(val).toEqualTypeOf<number>();
+        expectTypeOf(index).toEqualTypeOf<number>();
+        expectTypeOf(arr).toEqualTypeOf<ReadonlyArray<number>>();
+        return index < 5;
+      }),
+    );


There's actually a lot of cases where the piped dataLast version breaking without the dataFirst version breaking, let's split these tests

eranhirsch · 2024-05-01T06:37:45Z

src/every.test.ts

+      expectTypeOf(index).toEqualTypeOf<number>();
+      expectTypeOf(arr).toEqualTypeOf<ReadonlyArray<number>>();


We don't need to test these, it's part of the function signature, and they aren't inferred or have any "magic" that might break them.

Suggested change

expectTypeOf(index).toEqualTypeOf<number>();

expectTypeOf(arr).toEqualTypeOf<ReadonlyArray<number>>();

eranhirsch · 2024-05-01T06:38:06Z

src/every.test.ts

+        return true;
+      }),
+    );
+    expect(data).toBe(true);


runtime concern in a type test

eranhirsch · 2024-05-01T06:47:53Z

src/every.ts

+export function every<T, S extends T>(
+  data: ReadonlyArray<T>,
+  predicate: (input: T, index: number, array: ReadonlyArray<T>) => input is S,
+): data is Array<S>;


The typing does not respect the shape of the input in the output. We can take the tuple as a generic parameter and use it to return a result that is the same shape.

Suggested change

export function every<T, S extends T>(

data: ReadonlyArray<T>,

predicate: (input: T, index: number, array: ReadonlyArray<T>) => input is S,

): data is Array<S>;

export function every<T extends IterableContainer, S extends T[number]>(

data: T,

predicate: (input: T[number], index: number, array: T) => input is S,

): data is { [K in keyof T]: S };

This would allow someone to test something like [string | number, ...Array<string | number>] and get back [string, ...Array<string>]

The non-narrowing overload doesn't need this treatment because it returns a boolean

eranhirsch · 2024-05-01T06:48:37Z

src/every.ts

+export function every<T, S extends T>(
+  predicate: (input: T, index: number, array: ReadonlyArray<T>) => input is S,
+): (data: ReadonlyArray<T>) => data is Array<S>;


Same as above

Suggested change

export function every<T, S extends T>(

predicate: (input: T, index: number, array: ReadonlyArray<T>) => input is S,

): (data: ReadonlyArray<T>) => data is Array<S>;

export function every<T extends IterableContainer, S extends T[number]>(

predicate: (input: T[number], index: number, array: T) => input is S,

): (data: T) => data is { [K in keyof T]: S };

Brodzko · 2024-05-01T16:19:13Z

Sorry for the comments so late in the development. I've been working mainly on V2 so didn't have time to check other stuff. Because the release of v2 is happening very soon and it's likely we'll merge these changes only after releasing v2 it needs to match the style for v2.

No worries. How soon are we talking? If preferred, I can wait for v2 to release and then re-open this with all the things we found during this review resolved

eranhirsch · 2024-05-02T10:31:16Z

It should happen in the next 2 weeks

Brodzko marked this pull request as draft April 10, 2024 21:27

Brodzko mentioned this pull request Apr 14, 2024

Add some and every? #624

Open

eranhirsch changed the title ~~Draft: feat(every): Add every~~ feat(every): Add every Apr 14, 2024

Brodzko marked this pull request as ready for review April 14, 2024 12:57

cjquines reviewed Apr 16, 2024

View reviewed changes

Brodzko requested a review from cjquines April 19, 2024 22:24

Brodzko force-pushed the add-every branch from 3f4661a to e06c435 Compare April 19, 2024 22:26

Brodzko mentioned this pull request Apr 19, 2024

feat(some): Add some #646

Open

4 tasks

cjquines approved these changes Apr 22, 2024

View reviewed changes

cjquines self-requested a review April 22, 2024 14:52

cjquines reviewed Apr 22, 2024

View reviewed changes

eranhirsch requested changes Apr 22, 2024

View reviewed changes

eranhirsch reviewed Apr 23, 2024

View reviewed changes

Martin Brodziansky added 11 commits April 28, 2024 00:14

Helper types for type predicates

902c523

Implementation of every

479a8c6

Tests for every

4f68c01

Improve tests

ab5d9dc

Add proper JSDocs

33d82a4

Export every from index

8d3fced

Add every to mapping

0971186

Sort every inside mapping.md

1b8e4ce

Remove note from every description and a test case for conditional in…

9d83837

… case it gets resolved

Remove redundant unit test

5040c56

Remove pipeable tag from every

d042c57

Brodzko force-pushed the add-every branch from e06c435 to d042c57 Compare April 27, 2024 22:14

Address PR comments

0dc1265

Brodzko requested review from cjquines and eranhirsch April 27, 2024 22:41

Another batch of PR comments

6dedb17

eranhirsch reviewed Apr 30, 2024

View reviewed changes

Inline types and pass predicate directly

64ca4ef

Brodzko requested a review from eranhirsch April 30, 2024 15:37

eranhirsch requested changes May 1, 2024

View reviewed changes

		\| [`entries`](https://remedajs.com/docs/#entries) \|
		\| [`toPairs`](https://lodash.com/docs/4.17.15#toPairs) \| [`toPairs`](https://ramdajs.com/docs/#toPairs) \|

	data.every((element, index, array) => predicate(element, index, array));
	data.every(predicate);

		@@ -0,0 +1,63 @@
		import type { PredIndexed } from "./_types";

		expectTypeOf(index).toEqualTypeOf<number>();
		expectTypeOf(arr).toEqualTypeOf<ReadonlyArray<number>>();

feat(every): Add every #631

Are you sure you want to change the base?

feat(every): Add every #631

Conversation

Brodzko commented Apr 10, 2024 • edited

Available Types:

codesandbox-ci bot commented Apr 10, 2024 • edited

codecov bot commented Apr 10, 2024 • edited

Codecov Report

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

cjquines commented Apr 16, 2024

Brodzko commented Apr 19, 2024

cjquines 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

cjquines 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

eranhirsch Apr 23, 2024 • edited

Choose a reason for hiding this comment

Brodzko commented Apr 27, 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 May 1, 2024

eranhirsch commented May 2, 2024

feat(every): Add `every` #631

feat(every): Add `every` #631

Brodzko commented Apr 10, 2024 •

edited

codesandbox-ci bot commented Apr 10, 2024 •

edited

codecov bot commented Apr 10, 2024 •

edited

eranhirsch Apr 23, 2024 •

edited