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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

orestis
Copy link
Member

@orestis orestis commented Mar 14, 2019

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
Copy link
Collaborator

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
Copy link
Member Author

orestis commented Mar 15, 2019

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

lilactown commented Mar 18, 2019

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
Copy link
Member Author

orestis commented Mar 18, 2019

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
Copy link
Collaborator

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
Copy link
Member Author

orestis commented Mar 18, 2019

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
Copy link
Collaborator

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!

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