Closed mindplay-dk closed 6 years ago
I think, in most systems that offer both a create and update notification, the initial creation triggers both a create and an update notification because there isn't typically much use for an update notification that doesn't fire the first time.
I've noticed sometimes you need to use both oncreate
/onupdate
for the same thing and sometimes don't. When you have to, it's inconvenient writing a function with some logic and calling it in two different places.
in most systems that offer both a create and update notification
Do you have a concrete example?
Also cc'ing this to @zaceno.
I've noticed sometimes you need to use both
oncreate
/onupdate
for the same thing and sometimes don't.
That's what I mean. I think this change would take care of that?
I guess it's a matter of what you think "on update" means - does it mean "updating the element", or "updating the attributes of the element"?
I think the latter makes more sense - because we don't really "update the element"; there's nothing you can update, other than it's properties and attributes, so we're really "updating the attributes of the element", which happens both after creating and updating the elements.
Can you think of a use-case where you need to do something when the attributes get udpated except the first time? I can't. Almost every use-case I can think is either creation only, or every update.
Can you think of a use-case where you need to do something when the attributes get udpated except the first time?
I can't either. 🤔
@JorgeBucaran @mindplay-dk
I haven't had situations where I need to do something on update except the first time.
What I have found, are usually I have to do the update-thing + something extra in oncreate.
I suppose it could work as well to have onupdate run also the first time, but there would have to be a predictable order, like: oncreate always runs before onupdate.
Except that will become tricky if, when the element is first created I want to do:
1) some stuff 2) the same stuff I want done every onupdate 3) some more stuff
In that case I'll have to move 2-3 into the onupdate, and use an If statement to detect if this is the first run of onupdate and only do 3 in that case.
This might not be very common though.
I can see how running onupdate also after oncreate would be quite practical in many cases. But personally I like the clear distinction we have currently: this happens the first time, and this happens all other times.
What do you think @SkaterDad?
I haven't used onupdate
for anything yet, so I'm not going to be opinionated here.
Having separate oncreate
and onupdate
hooks seems pretty logical. I wouldn't be opposed to running onupdate
directly after oncreate
, but @zaceno brings up some good points on where that could trip people up. If your oncreate
and onupdate
functions need to do the same thing, it's only a couple lines of code, so I'm not seeing a problem.
I can see how running onupdate also after oncreate would be quite practical in many cases. But personally I like the clear distinction we have currently: this happens the first time, and this happens all other times.
The most common use-case I can think of, is when integrating with a third-party JS widget - this will need to be updated every time, e.g. both when creating and patching.
So perhaps there's a use-case for another event that only happens when an existing element is patched?
So like:
onupdate
would fire every time the properties/attribtues of an element get updatedonpatch
would fire only when the update patches an existing element (current behavior of onupdate
)Make sense?
tbh, I can't see myself ever actually using onpatch
- the execution order problem described by @zaceno, like he said, probably isn't very common.
it's only a couple lines of code, so I'm not seeing a problem.
@SkaterDad yeah, but it's a couple of lines of code what seems to be the majority use-case - it's fine to also support minority use-cases of course, but in my opinion the order and scope of events should be tailored so they match the majority use-case.
The only way to figure this out is by coming up with an actual use case, whatever it may be.
The only way to figure this out is by coming up with an actual use case, whatever it may be
Well, one thing I'm trying to figure out is how to deal with components that need internal state - I'm almost positive we'll need the "on every update" hook for that.
So for example, I built this date-picker as an exercise on friday:
https://jsfiddle.net/mindplay/6czx4dmg/
It went smoothly, and I'm very happy - but the question is, where does internal state go? For example, _offset
in this model is the current +/- offset in months from the currently selected date, which is strictly interesting to this widget internally - no one will ever need to know about this state.
How can I turn this into a reusable component? Without forcing client views to deal with the irrelevant offset
value, e.g. by maintaining both the value (selected date) and the offset as state in their own model?
This is the big question for me right now.
I love building individual components in this manner, but it's no real use unless I can also turn a widget like this into a reusable component.
I've been trying to figure out how to use oncreate
, onupdate
and ondestroy
in practice to implement private component state, but haven't come up with anything so far.
I guess what I'm looking for is a "mimimum viable component" pattern with Picodom - not so much a feature or a framework, but just the pattern itself.
No doubt this is going to depend on an update hook that fires with every update, and as mentioned, so will integration with third-party JS widgets.
Those are currently the two hottest use-cases for me, as this is what will make it possible to scale to rich, complex UI and reusable widgets.
Any suggestions would be more than welcome :-)
@mindplay-dk That's awesome, but I mean what would be the use case for running code on onupdate
, but not on oncreate
or at least not the same code.
I was just thinking how in Hyperapp, where there is ssr re-hydration, oncreate
won't be called after re-hydration (since the vtree already exist) and only onupdate
is. Your proposal would not affect that behavior, just saying.
@zaceno What is a valid use case for running code only on onupdate
? Just wondering.
@JorgeBucaran I don't know of any use case for running code only on onupdate
- what I was saying before is rather that there are good use cases of this:
oncreate
does X + Y (or Y + X ... the order may vary) onupdate
does X@mindplay-dk Perhaps a bit OT, but you might be interested in seeing how I solved the problem of stateful components in hyperapp, here: https://github.com/zaceno/hyperapp-nestable. Yes it's hyperapp, but the same idea likely holds for picodom + whatever state manager
Actually not so OT come to think of it, because see the src: https://github.com/zaceno/hyperapp-nestable/blob/master/src/index.js ... It's actually a perfect example of oncreate
doing onupdate
+more.
It's also a good example of how things would be more difficult (with this solution anyway) if onupdate
ran when the element is created as @mindplay-dk suggested. Because it would mean:
1 ) I must be sure that oncreate
runs before onupdate
2) In onupdate
I need to check somehow if this is the first run of onupdate so I can do if (_actions.init) _actions.init()
(oh and I'd need to save the init function on the element too)
not terrible, and it could work. But this why I think I prefer things the way they are.
@mindplay-dk Regarding your suggestion of adding an onpatch
(which behaves like todays onupdate
, and make onupdate
behave like you suggested):
We experimented with your suggested type of onupdate
(only run when props change) but eventually went back to just every patch.
I think the reason was that it can be kind of hard to know (in picodom) if the props really were changed (we'd have to do deep comparisons, and functions are just impossible to tell) -- so better to just assume everything has changed and leave it up to userland to do the comparisons in an informed way. @JorgeBucaran can probably explain it better.
Anyway, I was happy to go back to the current behavior, because in hyperapp-transitions (which also work with picodom. I've added examples in my README.md
now) I need to listen to an event which would be called for every transitionable-element, every time the layout may have changed. So basically I want a callback whenever anything anywhere has changed -- i e, the current onupdate
behavior.
EDIT:
If we want to continue talking about the idea of onupdate
running only when properties has changed, I'm not opposed to that (as long as we keep something like @mindplay-dk 's suggested onpatch
. But that's a separate topic, so: new issue for that, I think.
Currently
oncreate
onupdate
@mindplay-dk's suggestion
oncreate
→ onupdate
onupdate
So, taking in @zaceno's comment, my current suggestion is:
oncreate
→ onupdate
onpatch
→ onupdate
In summary, oncreate
and onpatch
are mutually exclusive - "create" and "patch" are about elements being created or patched, while "update" is about properties/attributes being updated, which happens both when elements are "created" or "patched".
But that's a separate topic, so: new issue for that, I think.
I think we should deal with this change as one PR - updating the documentation will be important, and changing the behavior and description of onupdate
doesn't make sense (per @zaceno's comments) without also introducing and describing onpatch
.
Let's get the changes, documentation updates, and tests, into a single PR, so that the project doesn't end up in an intermediary state that doesn't make sense?
Or we could leave things the way they are.
@mindplay-dk I agree that any changes that belong together should be PR:ed together, but for just discussing how things should be, issues should be used (like here). And separate questions should be in separate issues or we'll have trouble reaching agreement on anything.
Now I realize I might have misunderstood you. First I thought you were suggesting that onupdate
would only run when any of the components props change. Like I said, we've tried that and it's a fraught topic. So if that was what you were suggesting it's a whole other can of worms worthy of a separate issue.
But (at least for this issue) you're just talking about the timing -- right? What happens the first time an element is created, and what happens every time the patch function is run for the element. Wether or not props have changed is not relevant. Is that correct?
@mindplay-dk @JorgeBucaran
I'm on board with @mindplay-dk 's latest suggestion -- that works for me. But I'm also OK with leaving things as they are.
First I thought you were suggesting that
onupdate
would only run when any of the components props change
To be clear: no, I'm suggesting onupdate
run consistently for every update of attributes, whether that update applies to a newly created or patched element, and whether or not any attributes have actually changed - because that seems to be the most common use-case.
For one, we'll need this functionality if we're going to integrate with third-party JS widgets - for example, a date-picker would need to be updated, always, whether the element was just created or is being patched.
Now, another thing occurs to me though - functional components do get invoked with every update, so the function itself also kind of works as a "hook" for every update. The problem with that one is, the component has no access to it's prior state, and no means of telling whether the current call will trigger oncreate
.
My main interest in this subject to begin with, is figuring out how to build reusable components. My date-picker example makes for a real use-case: how would you turn this into a component? I've been struggling to figure this out, and because the create/update/destroy hooks do not have access to any common context, I'm stuck with this one.
Is it possible with the current life-cycle hooks? Would love to see an example.
I don't want to introduce a new lifecycle event.
So, if:
oncreate
→ onupdate
onupdate
does not disrupt anything, I am probably okay with it. But I would like to make sure this does not break anything.
does not disrupt anything, I am probably okay with it. But I would like to make sure this does not break anything.
It's a breaking change, of course - if you were manually calling the same function from oncreate
and onupdate
, you'll need to fix that.
@zaceno can you look at your use-case and try to determine if it could be implemented with this change?
If so, I'd say, let's go ahead and change onupdate
- if there is no current use-case for a new onpatch
, we can cut it; if a use-case emerges, we can consider introducing it then.
It's a breaking change, of course - if you were manually calling the same function from oncreate and onupdate, you'll need to fix that.
That's okay, I can accept that. But will it make impossible to do something else?
But will it make impossible to do something else?
I just had an idea, let me just to try something out before we go ahead with this change - I want to check whether this might block a component implementation pattern I just thought of...
@SkaterDad @zaceno If we change this behavior, it will make it to Hyperapp as well, so I'd like to make sure (1) we want this (2) it does not prevent you from doing something else and (3) we won't end up introducing another lifecycle event later.
If we dont add a lifecycle method to behave like onupdate currently does, I am opposed to this change.
It Is a breaking change, for one. It doesn't make anything impossible to do afaik - but it does make at least one of my uses more cumbersome.
If @mindplay-dk is just trying to work out how to make stateful, reusable components, let's just help him with that.
The lifecycle methods do have one thing in common, upon which state (or a whole state management system) can be attached - namely the element.
@zaceno It Is a breaking change, for one. It doesn't make anything impossible to do afaik - but it does make at least one of my uses more cumbersome.
Could you explain your use case to me? I still don't get it.
If @mindplay-dk is just trying to work out how to make stateful, reusable components, let's just help him with that.
@zaceno thanks 😄 yeah, that is my primary pursuit at this point.
I just found an article here talking about how to attach state to a functional component in React:
https://medium.com/@dai_shi/attaching-state-to-stateless-function-components-in-react-db317a9e83ad
Unfortunately, the library relies on React Component to implement it:
https://github.com/dai-shi/react-compose-state/blob/master/src/index.js
I can't think of a way to preserve state between calls that doesn't involve either:
I wrote a bunch of example components on friday, using a pub/sub pattern where the model broadcasts a notification on model invalidation - then using a render-function to update the view.
I like this approach, but the next step is to make it reusable, preferably using JSX syntax, so, something like <DatePicker value={ model.start_date }/>
in your application, with the expectation that the date-pickers internal state will initialize and be preserved - internal state such as _offset
in my example, which designates the current offset from the selected date, in months, which is of no interest to the application itself; it wouldn't care about anything other than the selected date.
I don't understand how that can be done using functions only?
@mindplay-dk Why are we discussing this on this issue though?
@JorgeBucaran I'm in agreement with @zaceno, that altering the oncreate
behavior to also call onupdate
is a breaking change. I like the way it acts now, personally, since the lifecycle names tell you exactly when they'll be called.
@JorgeBucaran Could you explain your use case to me? I still don't get it.
See the link I posted above : https://github.com/zaceno/hyperapp-nestable/blob/master/src/index.js
Never mind what it does. Just try to rewrite it to have the same behavior with the suggestion of running onupdate
immediately after oncreate
and you'll see the problem. It's possible, but gets a lot more complicated, because you'd need to have some code in onupdate
which should only run if this is the first call to onupdate
-- and you need a way to know that.
EDIT: Actually sorry. You do need to mind what it does :P. the crux is this:
...
oncreate: function (el) {
...
el._$(props)
if (_actions.init) _actions.init()
...
},
onupdate: function (el) {
el._$(props)
},
So el._$(props)
sets the properties passed to a component on its internal state. That has to be called before if(_actions.init) _actions.init()
-- or else there won't be any initial state for the internal init action to work with.
So, if I leave it as it is, then with the proposed change, the order things would happen is this:
//in oncreate:
el._$(props)
if (_actions.init) _actions.init()
//in onupdate:
el._$(props)
If the init
action needs to do anything to the state-properties that were provided from the outside, those changes will be overwritten immediately by the second el._$(props)
So, I could change things to be:
...
oncreate: function (el) {
...
},
onupdate: function (el) {
const virgin = somehowFigureOutIfThisIsTheFirstUpdate(el, props)
el._$(props)
if (virgin && _actions.init) _actions.init()
},
... the trouble being, besides the conditional code, how to implement somehowFigureOutIfThisIsTheFirstUpdate
@mindplay-dk
I took your datepicker js-fiddle and turned it into two instances of a reusable datepicker component such as you wanted. Have a look: ~https://jsfiddle.net/1vhetmo5/4/~
NOTE: check the console also, for stateful components to be useful, you usually need something out of them, so I gave the component an onselect
callback, which the main app binds to console log, so you can see it works
NOTE 2: My bad, here's the up-to-date link with the onselect
behavior. https://jsfiddle.net/1vhetmo5/6/
@zaceno thanks, but this is what I suggested not to do in (2)
Using a class/instance for the component model - the application model would need to create component models and then pass them to the view-functions, but, this involves creating and managing the model instance anyway, which doesn't seem any better/worse than class-based components
There are two reasons I don't like this approach:
You renderer
function is interesting - it's a lot like the mount()
function you find in various frameworks? You also managed to achieve individual rendering of component instances, which is very cool.
You're injecting the model into the DOM element, which may be problematic. I don't know if circular references still lead to memory leaks in modern browsers, but people invented things like .data()
in jQuery to avoid problems that could arise from this approach, I think?
I don't want to hand-code this kind of thing every time though, I don't think anybody does - that's probably why classes are popular? It would be too verbose and error-prone.
So maybe I am looking for something more framework-ish after all. Has anyone built anything on top of Picodom yet? I started something a while back (mainly to get support for class={{ foo:true }}
) but I didn't publish anything. Perhaps it's time to start talking about a "plus" layer as a separate package, with support for stateful components, a mount()
function, etc.?
It seems we are no longer talking about the original issue anymore. And it seems the conclusion we reached is to add more lifecycle events or keep things the way they are, so I am going to keep things the way they are for the time being.
@mindplay-dk Has anyone built anything on top of Picodom yet?
I found a few dependents of Picodom. I don't know how actively maintained those projects are, but maybe someone already figured out stateful components they way you envision them.
@mindplay-dk thanks, but this is what I suggested not to do in (2)
Hehe ... you wrote that model as a class, not me 😄
I wouldn't have used classes either but I was just trying to do "minimally invasive surgery" to demonstrate the technique. The key part is how I implemented the DatePickerComponent
. Basically, it's like a mini app attached to the main app.
Yes that involves binding the state-manager (instance of DatePickerModel) to the element. I can't think of any better way and I'm pretty sure it's safe (unless you do something wicked with eventlisteners) -- but if you find a concrete problem with it be sure to let me know.
BTW from the list of picodom dependents @JorgeBucaran linked to, I know that frapp
is maintained and capable of stateful components in some form (basically the same idea as my implementation above: composing many small apps into a bigger app). But not much documentation from the looks of it. Still better than my entry zx-app
which has zero docs 😆 . ( But that's intentional... it works well, and you can look into the code if you like, but it's not for public consumption just yet)
Hehe ... you wrote that model as a class, not me 😄
To be clear, I don't have a problem with classes 😃
But if the solution involves a class (or any object model) the framework needs to somehow manage the life-cycle of the object internally - it should be an implementation detail, not something that forces the consumer of the component to deal with the model and view independently. What makes this error-prone (and confusing) in my opinion, is the need to manually wire together two ends of the same thing - the component must be readily usable.
Here's another possible approach I just thought of: the call to the functional component itself has access to props
, which may be a better option than injecting state into the DOM element - if we inject something into props
, it should carry over via oldProps
which is made available to onupdate
, which can then forward inject it into the new VNode.
Creating a general facility like this one might then be possible??
@mindplay-dk Have you considered joining Hyperapp's slack? There's an entire room dedicated to picodom.
@JorgeBucaran there's a slack? There's no link to it anywhere 😆
By no means "there" yet, and I have to quit for today, but I did make some interesting progress.
https://jsfiddle.net/mindplay/o2rdL259/4/
I've extended the h
function so I can test if type
is a constructor that extends Component
- right now it's recreating the component on every iteration, but so far so good. Next step is to try to inject the component instance into a reserved field in props
, and make it carry over via oldProps
, which should be possible by proxying oncreate
and onupdate
.
The sample class here illustrates what I'm trying to do - I think that class-based components do make sense here - you'd be able to encapsulate the model and the render function into a self-sufficient, reusable component. It's not exactly the React approach, as I'm putting the component itself in charge of managing it's own state and calling Component.invalidate()
to re-render. We'll see if this pans out... :-)
@mindplay-dk Yes (the picodom channel is new, hence no link), and you are welcome to join! 🍪
Hmm, okay, I don't think class-based components are possible with what we have now.
The problem is, patch
only lets you patch the entire contents of an entire element.
Components can occur anywhere in a list of child elements, and need to render invidually.
The oncreate
and onupdate
hooks aren't invoked until rendering actually happens, so the Component class, during the call to h
, can't know whether it should construct a new instance or create a new one.
So the only possible work-around right now, is to wrap every component in a dummy div, which is obviously not the way to go.
Short of forking and modifying patch
to actually support calls to a render
method, I can't think of a working approach - since patch
is internally recursive, proxying patch
doesn't make any sense and won't work. (you'd have Component support only during the immediate call.)
I mean, there's one approach, which is to replicate the entire diff/patch algo and recursively handle Components before calling the actual patch-function, but that hardly makes any sense?
Hey mindplay, you need to move the state into the component. Talking about state getters and setters. On top of that, state needs to be in another semi-private class property that the accessors use. That way this gets inherited by all extensions of the Component class. You would also want to store a reference to the parent, oldNode elements directly on the Component class. That way each instance has its own reference to its parts to use when patching. And finally, patching is done with setting state. I put together this Codepen to illustrate: https://codepen.io/rbiggs/pen/opqyEM?editors=0010. With this approach, when you create a stateful component and instantiate it, it automatically gets rendered to the DOM. Also updating its state will patch the DOM again. Missing, add a setState
method that can accept either data or a callback that gets the component state.
This is very basic but shows that you can create class components that work with Picodom without a problem. This gives you basically a React-lite. Having component instances handle patching makes it easy to create multiple instances of a component with different state.
Thanks, @rbiggs. Long time no see! 👋
I am going to ping @mindplay-dk just to make sure he gets this notification.
@rbiggs yes, this is what I've been doing, more or less.
https://jsfiddle.net/mindplay/o2rdL259/
I use a simple pub/sub to decouple the model from the view, but this is otherwise similar.
This works great for individual pieces of UI, but it doesn't work well for composite views - and it doesn't enable JSX syntax for components, you have to manually manage the life-cycle of each component.
We're discussing it further here.
With Preact, Inferno and React, when a component first renders only componentWillMount
and componentDidMount
fire. After that, when the component is updated componentWillUpdate
, componentDidUpdate
and componentWillReceiveProps
fire.
It doesn't make sense to me that you would expect an update event to fire when a component is first created. It's not being updated, it's being created. Two very different scenarios. Firing update when a component gets created feels like muddying up what update is. Update should be for when there's a change to the current state of a component. If the component doesn't exist yet and is being created, then there is no old state to update.
This brings me to another thing I've noticed with Picodom: onupdate
gets fired event when the component doesn't actually update. This happens when you rerender a component with the same data. In the updateElement
function you check to see if the old and new properties match before updating an element:
if (props[i] !== oldValue) {
setElementProp(element, i, props[i], oldValue)
}
But then, event when you short-circuit this be exiting when the props are the same, you still push onupdate
to the callback stack:
if (props.onupdate && shouldUpdate) {
callbacks.push(function() {
props.onupdate(element, oldProps)
})
}
Seems like this is weird. Why do you execute the onupdate
callback with every call of updateElement
but check to see if the props are different before actually updating an element?
I would not expect an onupdate
callback to be executed when the element wasn't actually updated. You could add a check in there, something like shouldUpdate
:
function updateElement(element, oldProps, props) {
var shouldUpdate = false
for (var i in copy(oldProps, props)) {
var oldValue = "value" === i || "checked" === i ? element[i] : oldProps[i]
if (props[i] !== oldValue) {
shouldUpdate = true
setElementProp(element, i, props[i], oldValue)
}
}
if (props.onupdate && shouldUpdate) {
callbacks.push(function() {
props.onupdate(element, oldProps)
})
}
}
With this check, onupdate
would only get pushed to the callback stack when the element actually gets updated. I dunno. Maybe I have too much coffee in my system tonight. ☕️👨🏻💻
@rbiggs onupdate will always fire, but we expose the oldProps so you can figure out if something actually was updated or not.
@rbiggs we actually had onupdate
working like you suggested for a while, but it didn't work out well.
For one, there might be reasons (arguably mischievous, but anyway) to want a hook/callback to do something in a component any time anything anywhere changes.
For two, since you'll mostly be writing your event handlers like onclick: ev => actions.buttonClicked(),
-- that means function instances will be new every time. And there's no really good way to check equality of functions when they have different instances. So in practice, most components (though not all) ended up getting onupdate
:ed anyway.
Basically: the user is better equipped to state the conditions of when the onupdate should be run, than hyperapp can be. Hence @JorgeBucaran 's suggestion above.
Another (even better) trick you could use is the fact that patching is entirely skipped over if a vnode is the same instance as the old one. That means you could memoize a component based on the props passed to it. If the props haven't changed you return the vnode in the memo (same instance as before) and no patching and no lifecycle callbacks are called for the entire subtree. (It's kind of like our version of React's shouldComponentUpdate
)
I noticed that
onupdate
doesn't fire during the first update, is this intentional?I think, in most systems that offer both a create and update notification, the initial creation triggers both a create and an update notification, because there isn't typically much use for an update notification that doesn't fire the first time.
Typically the notifications you need are for creation, for destruction, or for all updates - rarely does anyone need a notification for "all updates except the first one".
Thoughts?