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

feat(core): add distinctUntilChanged operator #58

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tamazlykar
Copy link
Contributor

No description provided.

@nx-cloud
Copy link

nx-cloud bot commented Jul 16, 2022

☁️ Nx Cloud Report

We didn't find any information for the current pull request with the commit f687137.
You might need to set the 'NX_BRANCH' environment variable in your CI pipeline.

Check the Nx Cloud Github Integration documentation for more information.


Sent with 💌 from NxCloud.

@raveclassic
Copy link
Owner

Hey @tamazlykar, what is the reasoning behind adding this operator? Property's are designed so that value comparison is an implementation detail and I wouldn't want to expose any kinds of "comparators" to the end user. By design, values are always compared by reference, if you need deep comparison this usually means the structure is mutated. Any kinds of mutations and similar impurities are out of scope of frp-ts

@raveclassic
Copy link
Owner

Ok, I see you need deep comparison due to different object literals (different references) even their contents are the same. Well, I can't say such things are in the spirit of frp-ts as the intention is clearly to set the new reference and deep comparison looks like a "fix" for that. Instead I'd suggest moving the comparison out of the set to a condition wrapping that set.

const a = newAtom({ name: 'foo' })
a.set({ name: 'foo' }) // the intention is to push new object

if (!deepEqual(a.get(), {name: 'foo'})) { // the intention is to check deeply
  a.set({ name: 'foo' }) 
}

@raveclassic
Copy link
Owner

ok found the original issue, linking #59

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 this pull request may close these issues.

2 participants