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

domFor: always get generation from delayedRemoval instead of parameter #3007

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kfule
Copy link
Contributor

@kfule kfule commented Feb 1, 2025

Description

The generation of domFor is no longer passed as a parameter.
This allows domFor to work well in onbeforeremove and onremove and reduces the amount of code.

Motivation and Context

#3005 did not properly take into account that domFor is public API.
This PR fixes these problems (#3005 (comment)):

  • The domFor in onremove() does not work well during delayed removal. (flems)
  • When there is onbeforeremove() in both vnode.state and vnode.attrs, domFor in onbeforeremove() in the latter vnode.attrs does not work well (since 3005.) (flems)

How Has This Been Tested?

npm run test including new tests

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • My change requires a documentation update, and I've opened a pull request to update it already:
  • I have read https://mithril.js.org/contributing.html.

@kfule kfule requested a review from a team as a code owner February 1, 2025 06:13
Copy link
Member

@dead-claudia dead-claudia left a comment

Choose a reason for hiding this comment

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

I'd like to see tests confirming multiple generations are tracked correctly, for completeness.

Given you do this:

  • Render vnode A with onbeforeremove and DOM nodes A1 and A2
  • Remove vnode A, do not resolve onbeforeremove
  • Render vnode B with onbeforeremove and DOM nodes B1 and B2
  • Remove vnode B, do not resolve onbeforeremove
  • Render vnode C with onbeforeremove and DOM nodes C1 and C2
  • Remove vnode C, do not resolve onbeforeremove

I'd like to see confirmation of this (6 possible removal sequences in total):

  • m.domFor(A) returns [A1, A2] consistently before A's onbeforeremove resolves, even if B's and/or C's resolves first
  • m.domFor(B) returns [B1, B2] consistently before B's onbeforeremove resolves, even if A's and/or C's resolves first
  • m.domFor(C) returns [C1, C2] consistently before C's onbeforeremove resolves, even if B's and/or C's resolves first

@kfule
Copy link
Contributor Author

kfule commented Feb 1, 2025

@dead-claudia Thanks for your review comments.
I have added the following tests as you commented.

  • m.domFor(A) returns [A1, A2] consistently before A's onbeforeremove resolves, even if B's resolves first
  • m.domFor(A) returns [A1, A2] consistently before A's onbeforeremove resolves, even if B's and C's resolves first
  • m.domFor(B) returns [B1, B2] consistently before B's onbeforeremove resolves, even if C's resolves first
  • m.domFor(B) returns [B1, B2] consistently before B's onbeforeremove resolves, even if C's and A's resolves first
  • m.domFor(C) returns [C1, C2] consistently before C's onbeforeremove resolves, even if A's resolves first
  • m.domFor(C) returns [C1, C2] consistently before C's onbeforeremove resolves, even if A's and B's resolves first
    • (Other patterns should have been implemented in the above test cases.)

I am a little sorry that the test codes are somewhat verbose.
It seems that the test codes could be more aggregated, but I did not want to make it too complicated.

@kfule kfule requested a review from dead-claudia February 1, 2025 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants