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

loosing state since commit 78d4204a9a52a826bd2be4e7383d29e04233fb5b #2654

Closed
maurerdietmar opened this issue May 3, 2022 · 6 comments · Fixed by #2673
Closed

loosing state since commit 78d4204a9a52a826bd2be4e7383d29e04233fb5b #2654

maurerdietmar opened this issue May 3, 2022 · 6 comments · Fixed by #2673

Comments

@maurerdietmar
Copy link
Contributor

I create VTag manually using the builder pattern.

This worked unless commit 78d4204.

My Vtag contains 2 children:

VTag { inner: Other { tag: "div", children: VList { children: [VComp { type_id: TypeId { t: 18257190933183877322 }, node_ref: NodeRef { references: None }, mountable: "..", key: None }, VComp { type_id: TypeId { t: 5180764447731008823 }, node_ref: NodeRef { references: None }, mountable: "..", key: None }], fully_keyed: false, key: None } }, listeners: Pending([]), node_ref: NodeRef { references: None }, attributes: IndexMap({Static("class"): Rc("pwt-fit pwt-panel")}), key: None }

I then append another child:

VTag { inner: Other { tag: "div", children: VList { children: [VComp { type_id: TypeId { t: 18257190933183877322 }, node_ref: NodeRef { references: None }, mountable: "..", key: None }, VComp { type_id: TypeId { t: 5180764447731008823 }, node_ref: NodeRef { references: None }, mountable: "..", key: None }, VComp { type_id: TypeId { t: 15011471947742392350 }, node_ref: NodeRef { references: None }, mountable: "..", key: None }], fully_keyed: false, key: None } }, listeners: Pending([]), node_ref: NodeRef { references: None }, attributes: IndexMap({Static("class"): Rc("pwt-fit pwt-panel")}), key: None }

This results in a complete redraw, i.e. the first 2 children gets re-created, so they loose all state.

Please note that this does not happen when using the html macro, because the html macro always generates the same number of children (using an empty VLIst for optional children).

@WorldSEnder
Copy link
Member

I notice that you don't use keys. Does it work with them? You'd need them for semantic guarantees like node identity.

@maurerdietmar
Copy link
Contributor Author

I notice that you don't use keys. Does it work with them? You'd need them for semantic guarantees like node identity.

VTAG VTag { inner: Other { tag: "div", children: VList { children: [VComp { type_id: TypeId { t: 9350230192792449941 }, node_ref: NodeRef { references: None }, mountable: "..", key: Some(Key { key: "toolbar" }) }, VComp { type_id: TypeId { t: 7481662576334678570 }, node_ref: NodeRef { references: None }, mountable: "..", key: Some(Key { key: "main_view" }) }], fully_keyed: false, key: None } }, listeners: Pending([]), node_ref: NodeRef { references: None }, attributes: IndexMap({Static("class"): Rc("pwt-fit pwt-panel")}), key: None }

Adding keys does not help ...

@WorldSEnder
Copy link
Member

WorldSEnder commented May 3, 2022

Adding keys does not help ...

The VList claims that it is not fully keyed (fully_keyed: false) which is probably a side-effect of using it's DerefMut implementation. Can you call recheck_fully_keyed() to recompute this fact or construct it with with_children/add_child/add_children - then it should say fully_keyed: true, after construction/pushing the children.

@maurerdietmar
Copy link
Contributor Author

The VList claims that it is not fully keyed (fully_keyed: false) which is probably a side-effect of using it's DerefMut implementation. Can you call recheck_fully_keyed() to recompute this fact or construct it with with_children/add_child/add_children - then it should say fully_keyed: true, after construction/pushing the children.

OK, it works with fuilly_keyed: true

But you cant expect that every child has a key?? At least it worked without in 0.19.3

@WorldSEnder
Copy link
Member

But you cant expect that every child has a key??

If you care about exact node identity, then yes, I do, for now. Feel free to contribute a smarter list reconciliation algorithm, the current one was changed slightly for performance reasons, which is why behaviour changed, but fully keyed lists were always required to get any benefit from keys at all and without them, it was never guaranteed to work, as far as I can see. It just did because the nodes lined up for the old alg or something like that.

All of this is moot when the vlist length doesn't change and items line up. Strictly speaking the exact behaviour is also not guaranteed, but I don't see why it would make sense to replace nodes in this case, for the unkeyed internal alg, in any situation, so it's safe enough to "trust" that implementation detail.

There are some actionable items though, I can see (in increasing order of difficulty):

  • Instead of exposing Vec<VNode> via Deref and DerefMut, expose that impl via a wrapper struct that borrows the VList and recomputes recheck_fully_keyed() on Drop. That should take care of having to manually do this part, and still allow direct access to a vec of children. Slightly breaking change, but easy enough to fix for users.
  • Introduce callback refs. Then at least you can redo your dom interactions when the underlying node changes. It's not optimal, but callback refs are planned anyway, I guess.
  • A reconciliation alg for VList that works with partially keyed lists. Would obsolete point 1.

@maurerdietmar
Copy link
Contributor Author

All of this is moot when the vlist length doesn't change and items line up. Strictly speaking the exact behaviour is also not guaranteed, but I don't see why it would make sense to replace nodes in this case, for the unkeyed internal alg, in any situation, so it's safe enough to "trust" that implementation detail.

It would be great if we can really trust on this behavior (instead of "trust" on an implementation detail).

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

Successfully merging a pull request may close this issue.

2 participants