-
Notifications
You must be signed in to change notification settings - Fork 8
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!: Add support for custom isEqual #59
Conversation
This got a lot more complicated than I like. |
src/index.test.tsx
Outdated
} | ||
|
||
{ | ||
// When subscribe does not support isEqual, the options is treated as default :'( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty unfortunate.
Maybe we should do a major release and not support all this crazy overloads?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, let's just do:
useSubscribe(r, query, opts?)
Where opts is def
, isEqual
, and deps
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the name default
over def
. I'm fine with deps
because dependencies
is so long but maybe it would be better to be more clear... especially if we consider dependencies to be very rarely used. I guess that convinced me. I will update the PR with the major breaking changes.
PS I've been thinking about https://github.com/rocicorp/replicache-react/pull/59/files#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80R122. I think we should make this API officially and document that If users wants these values to be reactive they can pass some deps instead that encapsulate the change that happened. My reasoning is that users ~never want these values reactive, as evidenced by the fact that nobody has ever complained about the fact that they are not. |
BREAKING CHANGE!
src/index.test.tsx
Outdated
} | ||
|
||
{ | ||
// This is invalid because the return type is not json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized this kind of advanced type checking could be put into replicache/reflect too.
I don't think it makes sense to have advanced type check here but not in replicache/reflect.
ad55a72
to
14cfb52
Compare
BREAKING CHANGE!
useSubscribe
now allows passing anisEqual
function that is used to compare the new returned value from the query with the last value. This requires Replicache 14 or a Reflect release containing acefbcefff649eed8adf5f06de345fee4f74f66c (at the time of writing no such release existed).The new API now takes an optional
option
object:This allows the query function to return non JSON values.
Note: If used with Replicache 13, the
isEqual
option is ignored which can be confusing.Note 2: If
isEqual
is omitted a deep JSON equality check is used which means that things likenew Set([1])
andnew Set([2])
are considered equal. Be careful!