Fork me on GitHub
#fulcro
<
2024-05-21
>
JAtkins06:05:09

I think I've discovered a bug in the StringBufferedInput (‼️ html inputs are so annoying to debug). I'm including a minimal reproduction in the thread below. The short story is that the internal :value prop on the component state is only updated by typing. If the model (props) change and make the input display a different number (1 in the example) the internal value isn't updated (it remains set to 2 ). When externally updating the input again to match the original value, the internal logic says that the last time it was typed in had the value (2) is the same as the new props from above, so nothing has changed (inputs.cljc:51). I'll probably have an idea how to fix this in the morning but I'm all ears for a solution if someone knows how to solve this without breaking anything else 🙂.

JAtkins06:05:46

(defsc InputComponentTest [this props]
  {:query         [:a/id
                   :act-v]
   :initial-state {:a/id  0
                   :act-v 1}
   :ident         (fn [] [:a/id 0])
   :route-segment ["hi"]}
  (dom/div
    {:style {:display        "flex"
             :flex-direction "column"
             :gap            "0.45em"
             :align-items    "flex-start"}}
    "State"
    (dom/code
     (dom/pre
      (pr-str props)))

    (dom/strong "Instructions")
    (dom/ul
    {:style {:paddingLeft "1em"}}
      (dom/li "Click `Set value to 1`")
      (dom/li "Observe the input and :act-v being set to 1")
      (dom/li "Clear the input and type `2`")
      (dom/li "Click `Set the value to 1` and observe that it is indeed 1")
      (dom/li "Click `Set the value to 2` and observe that it NOT 2, it still shows 1 though the state shows `2`"))

    (dom/div
      {:style {:display "flex"
               :gap     "0.5em"}}
      (dom/span "input")
      (f.inputs/ui-int-input
       {:value    (:act-v props)
        :onChange (fn [x] (mut/set-value! this :act-v x))
        :key      "key"
        :debug    "true"}))

    (dom/button
      {:onClick (fn [x] (mut/set-value! this :act-v 1))}
      "Set value to 1")

    (dom/button
      {:onClick (fn [x] (mut/set-value! this :act-v 2))}
      "Set value to 2")))

tony.kay13:05:07

The integer conversion is probably causing problems. That was never the best solution. Using sync transactions is the best fix, since that’s how React is intended to work (e.g. using !! versions of transactions).

tony.kay13:05:51

But if you find a way to make it better without that, I’m all ears. But like I said, using async transactions was always a hack since React can’t do the right thing with the focused input (cursor jumps) which is why the logic is there to avoid setting it from the incoming value if it already has the incoming value.

tony.kay13:05:41

Your video does seem to indicate that the cached value is perhaps behind one step for some reason…it is odd

JAtkins17:05:15

As far as I can tell, the stringValue is not updated if latestUserInputValue == latestValFromProps. Generally that's fine (e.g. when the model is nil while the user has changed the input from "" to "-" ) but when the input is controlled things become funky. Something along the lines of • User inputs value, latestUserInputValue = 1 and stringValue = 1 • Model changes from button click, latestValFromProps = 2 so stringValue is updated -> 2 • Model changes from button click, latestValFromProps = 1 which is the same as latestUserInputValue and bypasses updating the stringValue I'm thinking about adding a 4th value to the mix of cacheInvalid? - it mostly seems to work in my testing but I've got another (new?) niggling bug haha. BTW, I would prefer to use synchronous TXes, but this is a MVP of a different function in the app. The input is actually in a UI form and the buttons in the MVP are simulacrum for changing the entity being viewed. A possible solution to cache bust would be to change the react key of the owning object, but I'd prefer to fix the bug at this level if at all possible.

tony.kay17:05:44

I think the ideal solution would be to figure out how to find the real raw mutable DOM input, and check its value. If it current == incoming, then do nothing; otherwise set it

tony.kay17:05:56

the problem then is that React is a moving target on internal implementation

tony.kay17:05:21

BUT, with the recent addition of hooks, and by using ref on the input, I’m betting we could do exactly that

tony.kay17:05:40

remember that a lot of this code was written against react 15 or 16

JAtkins17:05:28

do you mean run a check (= (string->model (ACTUAL-STR ref)) incomingValue) ?

JAtkins17:05:46

I'm looking into that possibility right now btw

JAtkins21:05:58

It's probably possible but I made no progress on the matter. The tack I took was un-controlled input + ref hooks, but that ended up with infinite loops and other weird behavior

tony.kay15:05:02

Basically get a ref to the real input…then you can use the current value of that element to check current value vs incoming

tony.kay15:05:47

The problem with that is if the user is typing fast and we’re NOT using sync transactions, then mismatch is inevitable and we’d overwrite what they had typed ahead (lost characters). It’s kind of a crap problem

tony.kay15:05:04

I think this is why I implemented it the way I did…since state changes are always synchronous, you can track the “intended value” in local state, and then compare things when then “controlled” value comes in. But then the problem is you could have async lag where a sequence of events comes in “late”, so then you’re looking at comparing sequences of what the input looked like to what it looks like now.

tony.kay15:05:45

The “Official” support in React on inputs ALWAYS uses component-local state, because it’s the only way they can get it to work reliably given the mutable and browser-controlled nature of inputs

tony.kay16:05:59

this is part of the reason why I added sync transactions to Fulcro in the first place (but async is the default for bw compat). Inputs work better if user events are synchronous, so that you can always just “trust” the props value and ignore the state hack altogether.

JAtkins16:05:44

Yeah. Well, this bug isn’t affected by sync or async txes - I tried with both variations and it’s definitely not that haha. I tried rewriting string buffered input as a hook but I didn’t get very far after ~3h so I’ve shelved that idea for now since the pr modification does solve my bug in a logical way I think. I may revisit this in the future and try my hand at a hook enabled SBI again.

tony.kay16:05:29

k, thanks. If you have continued success with your patch, then I’ll apply it. These things are so touchy that I prefer not to take these in lightly, since they can break large swaths of other people’s code if not right

JAtkins16:05:02

Fair enough 👍