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

Fix a problem with NodeRefs and vtags, ref #2206 #2279

Merged
merged 1 commit into from Dec 20, 2021

Conversation

WorldSEnder
Copy link
Member

Description

Fixes #2206

This is in some sense a very poor-mans local fix. It works and has a test case - good.

There is something deeper to fix: every NodeRef should be owned by exactly one rendered vnode. If that invariant is violated, this could lead to unexpected results - like here: Complicating the situation is that during rendering, new nodes are mounted and refs are set immediately, while another part of nodes might get detached later during the same render - "owning" the same ref - leading to a time window where a NodeRef is "owned" by two nodes.

There might be other, similar bugs lying dormant until a bigger effort is spent on developing a conceptual understanding of the above and adding some more debug assertions to check for the implicit invariant. Note that for example, there are currently no checks for the user reusing the same NodeRef multiple times:

<>
  <div ref={&some_ref} />
  <div ref={&some_ref} />
</>

Which of those divs is expected to be in some_ref? Imo, some debug_assertion should trigger and inform the user not to do this in debug builds.

Checklist

  • I have run cargo make pr-flow
  • I have reviewed my own code
  • I have added tests

@voidpumpkin voidpumpkin added the A-yew Area: The main yew crate label Dec 17, 2021
Copy link
Member

@hamza1311 hamza1311 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hamza1311 hamza1311 merged commit 47364b6 into yewstack:master Dec 20, 2021
@WorldSEnder WorldSEnder deleted the fix-tag-refs branch December 20, 2021 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew Area: The main yew crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NodeRef unexpectedly reset when DOM structure changes in view()
3 participants