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

Support (or enforce) event callback keys #2

Closed
osener opened this Issue Jan 4, 2019 · 2 comments

Comments

Projects
None yet
2 participants
@osener
Copy link
Owner

osener commented Jan 4, 2019

I got bitten by OvermindDL1/bucklescript-tea#97 a while ago, looks like event callback keys are a crucial feature that needs to be supported in this PPX.

Reading this discussion, it seems like passing ~key to handlers such as onInput should almost be mandatory. Two ideas off the top off my head:

<input onChange={value => Msg} onChangeKey="foo" />

to make it optional, or

<input onChange={(value => Msg, "foo")} />

to make it mandatory by always expecting a tuple (breaking and maybe not obvious?).

@OvermindDL1 what would be your recommendation to solve this? If I'm remembering correctly, in my particular case it was before I passed key anywhere in the UI. Even without key or unique, the UI was getting re-rendered correctly, but the onClick handlers were referring to the initial value.

I remember fixing the issue by passing unique values to the parent container, but I was confused as to why everything seemed to work as expected without unique except the event handlers, if it's that crucial. Tea.Html.onClick doesn't even take a ~key argument, is that intentional?

In React, we pretty much only have to pass key when rendering a dynamic list of components or when we want to force a re-mounting of a stateful component, and this PPX allows making incorrect assumptions that could cause problems out of React habits.

@OvermindDL1

This comment has been minimized.

Copy link

OvermindDL1 commented Jan 4, 2019

Reading this discussion, it seems like passing ~key to handlers such as onInput should almost be mandatory.

They really really should have been, but Elm did not require them and I was trying to follow its API to ease porting (and Elm programs had similar issues as well), but I'm really leaning to making them mandatory...

Even without key or unique, the UI was getting re-rendered correctly, but the onClick handlers were referring to the initial value.

What 'should' be done is the key should somehow be some representation of the closed over data, even if it's just a simple counter that increments on every change of the closed over data, it just needs to be changed. It's hard to mandate that at a language level as you only know at runtime if they change. I've yet to come up with a good idea to 'bind' those together better since function closures in OCaml (and most languages, javascript as well) are always 'unequal'. I could go the all-out method and always update event handlers that don't have a key set but updating event handler in the DOM is quite slow, but even then that wouldn't catch it when someone forgets to update the key when the closed over data changes, or I could go nuclear and just always update the event handlers always, but that would be horrifying for performance.

if it's that crucial. Tea.Html.onClick doesn't even take a ~key argument, is that intentional?

That's because onClick takes a message, which is able to be compared at runtime so it should always work. The issue is purely surrounding closures/callbacks.

In React, we pretty much only have to pass key when rendering a dynamic list of components or when we want to force a re-mounting of a stateful component, and this PPX allows making incorrect assumptions that could cause problems out of React habits.

I've not really used React (I didn't come from the web world, rather the backend world, OCaml/C++/erlang/etc...) so input like this is very valuable to me. :-)

But from what I've debugged in to React doesn't register callbacks directly in the DOM to perform the action but rather just calls back to the instanced React view object (there are no such objects in Tea) and it itself is what handles the updating all wrapped around this so it is containerized. Essentially a Tea app conceptually runs like a single React Component, it's not a multi-tiered component (consequently you can actually mount a Tea app 'inside' a React component with the react component managing the lifetime of the Tea component).

If OCaml had metaprogramming ala Lisp or so then it would be trivial to make a singular unique message for every possible used message type and link that back directly in then there would be no issues, but without it the programmer needs to do a little bit more work.

Optimally all events would only be messages, not callback/closures. The only time a callback is really needed is to do some parsing of the event structure or to modify it in some way, and even then rarely is a closure needed then unless needing to, say, stopPropogation based on some program state (but then in that case the callback could be swapped out) then only a pure function callback without a closure would be needed and that would work again (as long as the key is changed when the function is changed, which is much easier to remember).

Maybe it would be better in this PPX to only allow Messages or a message constructor that accepts a value but with no closure at all, along with optional arguments to handle parsing and handling of the event structure? Any actual 'work' should only be done in update anyway so that would help to enforce that... Hmm... I really want to come up with a better idea for linking these... If only OCaml/javascript allowed for closure comparisons like a couple other languages do...

@osener

This comment has been minimized.

Copy link
Owner

osener commented Jan 5, 2019

Thanks for the very informative reply! I do rely on event.target in some places, but setting unique on the element itself does what I want (more I think about it, I was actually missing those and it makes sense). I'll follow what bucklescript-tea does about this, and maybe document it.

@osener osener closed this Jan 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment