Skip to content
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

Suggestion: comparer.shallow (or shallowArray) #1561

Closed
4 tasks done
xaviergonz opened this issue May 25, 2018 · 2 comments · Fixed by #2151
Closed
4 tasks done

Suggestion: comparer.shallow (or shallowArray) #1561

xaviergonz opened this issue May 25, 2018 · 2 comments · Fixed by #2151

Comments

@xaviergonz
Copy link
Contributor

xaviergonz commented May 25, 2018

Welcome to MobX. Please provide as much relevant information as possible!

I have a:

  1. Idea:
  • What problem would it solve for you? Sometimes I need to have reactions/computed values that change an array of immutable object references, so it would be nice to have a shallow array comparer
  • Do you think others will benefit from this change as well and it should in core package (see also mobx-utils)? It should be on the core package since that's where comparers are
  • Are you willing to (attempt) a PR yourself? Might

I have stumbled several times on a situation where I was generating a computed value that generated a new array of references to immutable objects. In this case the deep comparer had too much of a performance hit and was unecessary, so I had to use a function like this:

function shallowCompare(a: ReadonlyArray<any>, b: ReadonlyArray<any>) {
  if (comparer.default(a, b)) {
    return true;
  }
  if (!Array.isArray(a) || !Array.isArray(b)) {
    return false;
  }
  const aLen = a.length, bLen = b.length;
  if (aLen !== bLen) {
    return false;
  }
  for (let i = 0; i <= aLen; i++) {
    if (!comparer.default(a[i], b[i])) {
      return false;
    }
  }
  return true;
}

for this reason I think it could be a nice addition to the core comparers.

@xaviergonz
Copy link
Contributor Author

xaviergonz commented May 25, 2018

Or maybe it should just be added as part of the default comparer itself? I don't see any situations where it could be harmful (except for performance perhaps, but then in those cases the identity comparer could be used anyway)

@liuqiang1357
Copy link

liuqiang1357 commented Sep 6, 2018

comparer.structural won't compare the parts deeply which reference same object. so I think it won't have performance problem in most situation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants