Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider using something like fast-deep-equal to check if errors are equal in controlErrorChanges$ #131

Closed
jafaircl opened this issue Dec 9, 2021 · 0 comments

Comments

@jafaircl
Copy link

jafaircl commented Dec 9, 2021

Description

Comparing two objects using JSON.stringify has 2 drawbacks:

  1. It's much slower than it needs to be. Using a library that checks if two objects are equivalent based on their values can be about an order of magnitude faster. I forked fast-deep-equal and added equality functions using JSON.stringify and the stringify function from fast-json-stable-stringify to the benchmark. You can see some of the results below:
fast-deep-equal x 275,971 ops/sec ±0.57% (92 runs sampled)
fast-deep-equal/es6 x 241,026 ops/sec ±0.59% (92 runs sampled)
fast-equals x 282,790 ops/sec ±0.57% (88 runs sampled)
nano-equal x 199,804 ops/sec ±0.71% (93 runs sampled)
shallow-equal-fuzzy x 155,549 ops/sec ±0.50% (91 runs sampled)
JSON.stringify x 29,999 ops/sec ±0.73% (93 runs sampled)
fast-json-stable-stringify x 19,918 ops/sec ±0.55% (90 runs sampled)
The fastest is fast-equals
  1. If two objects have equivalent values but the keys are in different orders, they will be not be considered equivalent. For example, { required: true, min: true } and { min: true, required: true } are considered different values. With a library like fast-deep-equal or fast-equals, they would be considered the same.

Proposed solution

Instead of using the current equality function which uses JSON.stringify to check if two values are equal, use something like fast-deep-equal to check.

import fastDeepEqual from 'fast-deep-equal'

export function controlErrorChanges$(
  control: AbstractControl,
  errors$: Observable<ValidationErrors | null>
): Observable<ValidationErrors | null> {
  return merge(
    defer(() => of(control.errors)),
    errors$,
    control.valueChanges.pipe(
      map(() => control.errors),
      distinctUntilChanged((a, b) => fastDeepEqual(a, b))
    )
  );
}

I can create a PR to do this. But, it will add a dependency. So, I wanted to reach out and see if this is something you would be interested in changing before going any further.

Alternatives considered

We could pipe the errors$ observable like errors$.pipe(distinctUntilChanged(fastDeepEqual). But, that would still run them through the JSON.stringify equality function, negating the speed benefit.

Do you want to create a pull request?

Yes

@ngneat ngneat locked and limited conversation to collaborators Dec 10, 2021
@NetanelBasal NetanelBasal converted this issue into discussion #132 Dec 10, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant