nosco / hx

A simple, easy to use library for React development in ClojureScript.
MIT License
247 stars 16 forks source link

add an example and a possible solution for atomified state #38

Closed orestis closed 5 years ago

orestis commented 5 years ago

Related to #11, I've made a proof of concept of what something that uses refs could look like. The guarantee for refs is that they stay stable for the lifetime of the component, so sticking a proper atom in there seems to be working. The useEffect to add watch seems to be ok, and could potentially be a useLayoutEffect in a slightly modified version of this if needed.

Obviously I haven't tested this much further, so take everything I say with a grain of salt :)

lilactown commented 5 years ago

The thing that makes me wary of adding a proxy atom is that I think we might end up missing out on the scheduling stuff that React does for us when dispatching an update via an event handler.

When Concurrent React lands, renders will be marked with different priorities. React helps us out here:

(let [[state set-state] (react/useState)]
  [:input {:value state
           :on-change #(set-state (-> % .-target .-value))}])

The set-state update will be scheduled as UserBlocking by React since it's in the on-change handler. This routes around the common problem people have with e.g. Reagent, Rum, where controlled inputs are buggy due to using async rendering.

However, if that set-state call is triggered by some external event bus (e.g. an add-watch), I think it loses the priority and will be scheduled as Normal. Which causes us to run into the same issues as Reagent and Rum.

I'm basing this off of what I'm understanding by reading the React docs for the scheduler as well as this blogpost: https://philippspiess.com/scheduling-in-react/

This same issue actually exists with <-deref today, which I'm starting to fear might land us into trouble as well if we encourage people to use it.

orestis commented 5 years ago

Good thinking about concurrent react. I agree that any decision should be forward thinking.

I’m not 100% sure on the guarantees of add-watch, but if it is run “synchronously” with the swap!, woudn’t it be seen as running from inside the on-click handler? The useEffect is only to setup the watch, not to trigger renders.

lilactown commented 5 years ago

I think that's true; if watches are run sync then I think it should keep the schedule context the event was triggered.

Looking at the source for CLJS atoms, it looks like currently they are run sync.

lilactown commented 5 years ago

We should see if if we can come up with some test cases for concurrent React and set them up with diff solutions. I've been wanting to do the same for Reagent too.

lilactown commented 5 years ago

After some research and experimentation, the <-atom hook has different behavior when used in combination with closures like <-effect.

E.g. the example in Dan's blog post here: https://overreacted.io/a-complete-guide-to-useeffect/#each-render-has-its-own-everything

The <-atom has the same behavior as the class, where it doesn't capture the current value when it fires and instead logs You clicked 5 times, five times.

I've created a branch with a test for this here: https://github.com/Lokeh/hx/tree/atom-state

The test code starts here: https://github.com/Lokeh/hx/blob/master/test/hx/hooks_test.cljs#L139

orestis commented 5 years ago

Right, I guess that makes sense given the fact that an atom represents something mutable, so you pass the reference around, instead of the value.

I wonder if a better API for useState would move away entirely from the built-in swap! and reset! and instead look something completely different like:

(let
 [c (<-state 0 'foo) ; [foo setFoo] behind the scenes, nice for debugging, 'foo of course could be optional
  _ (state-> c inc) ; calls setFoo with inc
  _ (state->! c 5)] ; calls setFoo  5
[:div @c] ; c only supports deref, e.g. like a promise or future.
)

Optionally, both state-> and state->! could return the new actual value without waiting for react to do the rendering, but I haven't thought this through.

Or perhaps the useState API is just good enough, and there's no need to wrap it? Esp. since it looks like you can destructure JS arrays in let bindings.

lilactown commented 5 years ago

At this point I'm pro not wrapping it at all. I feel like any continued conflation with Clojure irefs is going to bring with it semantics that either aren't true or don't match the mental model React is pushing forward with.

orestis commented 5 years ago

I concur. Best to focus this library on hiccup and react interop, and use React hooks for everything else.

Wrapping react hooks with some tiny QoL functions is fine (e.g. converting a CLJS vector into a JS array for useEffect dependencies). Perhaps the <-atom and/or <-deref for Reagent compatibility and some CLJS specific stuff, in the form of custom hooks. But perhaps those could even be a separate library.

lilactown commented 5 years ago

Yeah, I think I will remove it.

I expect the next release of hx to be a major breaking change, removing the Atomified <-state hook and removing munging of props passed to components.

Will be interesting to see if it helps or hurts adoption 😝

Closing this for now. Thanks for walking through this with me!