[tracking PR] Render new #112
Conversation
It doesn't need to be a Component method, Markup-only would be fine if you'd prefer not to provide the option, shouldn't change the implementation in any significant way. The primary limitation of the current Keyer implementation is that all siblings must be keyed for it to work. I think from memory that React has the same limitation though.
Restorer can't work in its current state, for either re-rendering, or re-using components. As far as I can see, the only options to make it work are: require a Clone method; provide cloning via reflection (we could make this more robust than what's in this PR if it's the only path); or introduce some sort of explicit state implementation, where state must be managed via a separate object that can be compared - this takes us down a React-style path. The latter would be a less desirable path IMO, but has some merit.
Happy to help however I can, I will say that I've exercised the changes pretty well at this point. I'm not certain that merging the Mount/Unmount modifications without the later changes is worthwhile, since changes like deferring the lifecycle events reduces surprises significantly for some usage patterns. |
Yes, React has the same limitation: see https://facebook.github.io/react/docs/lists-and-keys.html |
Needed because any `(*wrappedObject).Call(...)` can produce an `js.Undefined` which in turn goes to `wrapObject` leading to a `TODO` panic. With this change, that does not happen. Also helps reduce #112
|
Rebased to account for 5ad2a3e |
…nt based on component equality Refs # 85 Fixes # 75 # 85
|
Rebased to account for #113 |
I agree that
If you could explain in further detail why you believe the I've also sent #114 just now which I believe would be all that is needed in order to make |
|
I'll keep the discussion here, because I don't believe #114 will help. The problem should be easily demonstrable with the following minimal example: type MyComponent struct {
vecty.Core
}
func (c *MyComponent) Restore(old vecty.Component) bool {
oldComponent, ok := old.(*MyComponent)
if !ok {
return false
}
if oldComponent == c {
panic(`Components point to same memory, change comparison impossible`)
}
return true
}
func (c *MyComponent) Trigger() {
vecty.Rerender(c)
}
func (c *MyComponent) Render() *vecty.HTML {
return elem.Div()
}What happens when Also, the panic in #114 on unrendered component seems arbitrary - I don't see why you can't just render as normal here, and if you can't it's probably because the old render code has other bugs. I guess maybe it doesn't make any sense to render here since there probably isn't a parent available, but panic seems like the nuclear option. 1 The term 'restore' for what the related code does is still backwards, I do hope we can change that. |
|
Understood, I see the problem you're talking about now! I very much appreciate the example.
A couple of things:
I'll circle back here on this at a later point, as I would like to discuss history here & potential better naming. In the meanwhile, I will update and merge #114 with something close to your |
NEW: - `Keyer` interface provides support for efficient list element re-use. Components simply need to implement the `Key` function, or the `vecty.Key` Markup may be used for HTML. BREAKING: - `Restorer` interface is now `Updater`, and its `Restore` method is now `ShouldUpdate`. The previous `Restorer` implementation was variously broken in any case, and poorly named. Additionally, it requires a `Clone` method to support passing a previous Component instance to `ShouldUpdate`. There is a limited generic version available on `vecty.Core` using reflection that may satisfy for users that do not wish to implement it themselves. - `Context` method on `Component` interface is no longer public. - `Restore` method on HTML is no longer public. Move DOM manipulation to HTML, and fix incompatible elements Fix for rendering with prevKeyed <-> nextKeyed transition Style fixes Avoid unnecessary element replacement on Rerender for compatible tags Defer lifecycle events until all mutations have been performed. This allows Mount handlers that depend on sibling or parent elements to function as expected, and for performing tasks like detecting element sizes. On double-mount, perform unmount first. On double-unmount, skip event. Add jsUndefined and fix lint Avoid panic on HTML.node == nil Skip Rerender for Components that have been removed
Batching makes updates smoother, and dedup avoids unnecessary mutations, since Component state must be maintained internally.
|
|
I think the best way to do it is to make all changes against While working on the
I assume that the reason this change was made is because If so, there is a lurking problem here which can't be shown by the todomvc code currently but would happen in the real world. In fact, it's the problem that
I have a feeling this is also why you believe |
Part of my goal here was to break up the work into units that are easier to reason about, to remove redundant code paths, and remove passing of redundant arguments. I think it may be difficult to do this piecemeal. I'd be happy to try and schedule a VOIP call of some variety to talk things through if you think that may help (scheduling may be slightly challenging, as I expect some time-zone conflict, but I'm sure we could work something out if you're interested).
Correct, the internal state is already updated via the input callback, so performing this action inside The Rerender in onNewItemTitleInput was removed because it's unnecessary.
I understand the scenario you've described, and based on this - To put this interaction in terms of React semantics, we're talking about a couple of things - Props (controlled by the parent Component), and State (maintained internally to the Component), and that for Vecty in the case of children that are newly instantiated every parent render, State in the absence some sort of Restore mechanic will be lost on parent Rerender, and Props will be reset to the parent's view. I think in React this works because there's essentially transparent state persistence between Components of the same type at the same location, and a child is only ever updated if its Props are changed by the parent or its State changes internally, whereas Vecty requires all children to be re-rendered for every parent render, and if instantiated inline, two Components of the same type, at the same location, are separate entities that need to be manually migrated. The React model looks more like how Vecty behaves (with the modifications to allow it) if child components are persistent references (ie, a single component pointer is re-used between renders), since then the child component is able to maintain internal state between renders. This is probably why I haven't come across the scenario described here - using my PoC router implementation naturally lends itself to this sort of pattern.
Whilst this may be part of what Restore is meant to do, this is not what reconciliation does in React, and it's not what doRestore and friends do in Vecty - they're responsible for mutating elements from one version of a DOM representation and the next, based on the current known state (which in React is maintained via separate mechanisms). In React, this is made easier by virtue of Props and State being explicit (and separate) entities, which allows the lifecycle triggers/events to resolve in sane fashion. Based on the use-case for Restore described above: In React terms, calling Rerender is closest to calling The problem with this pattern is that it makes it difficult for the user to reason about whether there has been a change of state or not, and there's pretty much no way for a component to determine whether it should actually copy values from the previous component to a new instance, since it can't differentiate between field values passed down by the parent at instantiation time; values that should be inherited from the previous instance (due to Go's zero values); and local state changes. I can think of a few approaches to handling this, but I'd have to examine them more closely, and some off the top of my head may require reflection, which I get the impression you're strongly against? Though I'm not sure what starting point I'd use at the current stage... also happy to work through ideas here if you have any. PS - Please excuse the Friday afternoon brain-dump - it's been a long week, and I wanted to get this down so I can come back for a refresher when I have time to consider this more closely. |
- The `Restorer` interface is now just `Restore(Component)` (previously `Restore(Component) bool`). - A `RenderSkipper` interface is added, where `SkipRender(Component) bool` replaces the old `Restore` return value behavior. A notable change here is that the `prev` argument to `SkipRender` is a `Copy` of the `Component` at the time of the last `Render`, while the `prev` argument to `Restore` is not as it needs to copy component state (struct fields) from the old component instance to the new one. See #112 (comment) for background.
|
Thanks for the writeup! I'm heading to sleep soon, so I haven't fully comprehended it yet & will say that I need to look more into the React differences you mentioned. But I did skim over it. I agree with what you said about I've spent the last few hours investigating this issue (again, against the master branch) and looking for a correct fix. What I've come up with is in #114 and I think it's the right approach. Essentially, with this change, we keep Hopefully that's a clear enough description for now, as I've gotta get to sleep. Check out #114 for more info or leave questions/comments there. I'll probably move forward with this tomorrow after testing with @shurcooL's Go Package Store application if there are no objections EDIT: also I return to work Monday, so I'm trying to move as quickly as I can to resolve everything here before then :) |
This is the first approach that came to mind for me too, but it doesn't quite cover all the usage problems. I'm heading out shortly, but I'll comment on #114 when I have a sec, and will try to set aside some availability over the next couple of days to help with whatever else. |
- The `Restorer` interface is now just `Restore(Component)` (previously `Restore(Component) bool`). - A `RenderSkipper` interface is added, where `SkipRender(Component) bool` replaces the old `Restore` return value behavior. A notable change here is that the `prev` argument to `SkipRender` is a `Copy` of the `Component` at the time of the last `Render`, while the `prev` argument to `Restore` is not as it needs to copy component state (struct fields) from the old component instance to the new one. See #112 (comment) for background.
|
@slimsag just FYI, today is the last day of the weekend for my timezone (GMT+10), so I can make some time available if there's anything you need me to do. |
|
Time to finally put this one to bed 🛌 |
|
🎉 thanks for confirming that! And thank you for working with me to land each of these changes individually, I really really appreciate it! Let me know if I can buy you a coffee or something :) |
|
It was a worthwhile process - I know we've found some cases together that I wouldn't have otherwise. Still more to do, but it's been a good experience. |
@pdf I've rebased your branch here (prior PR #107), while I try to decipher the rest of your changes.
I've dropped the prior commit for namespaced elements, in favor of the now-merged #110
I've squashed a number of commits into
Refactor rendering for reliability and performance.since they were not individual changes really.My plan:
Unmountchange againstmaster.Mountaddition againstmaster.master.Things I have doubts about:
Markup.Restorerinto two separateUpdatermethods is a good idea.Clonewould be a mistake, currently.