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

CLJS data structures are compared by ref in <-effect deps #39

Closed
lilactown opened this issue Mar 17, 2019 · 11 comments
Closed

CLJS data structures are compared by ref in <-effect deps #39

lilactown opened this issue Mar 17, 2019 · 11 comments
Labels
bug Something isn't working

Comments

@lilactown
Copy link
Collaborator

This can lead to during the effect more often than desired, since it does not use Clojure's equality semantics.

@lilactown
Copy link
Collaborator Author

Solution is to capture deps in a ref and do the equality check in clojure, pass result to React

@lilactown
Copy link
Collaborator Author

Fixed via ba0d520

@lilactown lilactown added the bug Something isn't working label Mar 18, 2019
@orestis
Copy link
Member

orestis commented Mar 19, 2019

I was just reading up on this, came to post about it :)

Doesn’t useState have the same issue? React will skip the render if the returned value is the same as the previous one, but it uses the same algorithm.

@lilactown
Copy link
Collaborator Author

Ah, I didn’t think to check useState as well. Good call out!

I wonder if there is a good way to handle that... I’m not sure how to do the comparison if the value given to the update function is itself a function to be applied, without doing the computation twice.

And using a ref we might end up creating similar bugs as with atomified <-state.

@orestis
Copy link
Member

orestis commented Mar 19, 2019

I’ve commented on that react issue as pointed by Roman in Twitter:
https://twitter.com/roman01la/status/1107695745493356545

facebook/react#14476

Some similar efforts to bridge Hooks and ClojureScript are here:
https://github.com/mhuebert/chia/blob/master/view/src/chia/view/hooks.cljs

@roman01la
Copy link

Hello! Do you have any ideas how this can be fixed for useState? I'm starting to think that Hooks API stands in the way of integrating React with ClojureScript when trying to provide idiomatic Clojure API for library users.

Including @mhuebert in this discussion, since he's working on a wrapper as well.

@orestis
Copy link
Member

orestis commented Mar 19, 2019

I don't have any specific thoughts in mind yet. I am working though on a real-world application that's built entirely on hx, so I'm using that as a real-life place to validate new ideas.

The thing that is really great with hooks though is that the API surface is rather small, so it's easy to write different wrappers and see how they behave. Their composability is what makes me want to keep pushing this, since doing the same with class-based components was pretty much impossible.

@roman01la
Copy link

roman01la commented Mar 19, 2019

Actually I think this is an easy fix for useState, if you wrap the hook you can compare values inside of the wrapper before setting a new value onto the hook roman01la/uix@3665c63

@orestis
Copy link
Member

orestis commented Mar 19, 2019

Yeah, that's the obvious fix. We discussed at length with @Lokeh whether it makes sense to expose the "ref" semantics of atoms, because they can be confusing if you're expecting normal clojure atom behaviour. See #11 (comment)

@mhuebert
Copy link

   (when (not= new-value value)
    (set-value new-value))

I was trying to think of values for which Clojure would say not= while Object.is would say true (ie. cases where set-value would be an undesired no-op), but the only thing I could think of is js/NaN, which is probably not an edge case to worry about.

@orestis
Copy link
Member

orestis commented Mar 19, 2019

I think that's a very good point and a test suite could be setup to explore this. If it's only js/NaN then perhaps it's not worth the hassle :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants