Skip to content

fix: correct inner effect cleanup order - #62

Open
YuanchengLi-Kevin wants to merge 1 commit into
littensy:mainfrom
YuanchengLi-Kevin:fix/inner-effect-cleanup-order
Open

fix: correct inner effect cleanup order#62
YuanchengLi-Kevin wants to merge 1 commit into
littensy:mainfrom
YuanchengLi-Kevin:fix/inner-effect-cleanup-order

Conversation

@YuanchengLi-Kevin

Copy link
Copy Markdown

Addresses issue #60 with owner's suggested fix.
TLDR: Clean up child effects and effect scopes before their parent reruns, and dispose dependencies in LIFO order.

New

Updated

  • run(): check HAS_CHILD and unlink child effects before parent rerun
  • stopEffect(): integrate disposeAllDepsInReverse()
  • unwatched(): integrate disposeAllDepsInReverse()
  • effect(): set HAS_CHILD flag
  • effectScope(): set HAS_CHILD flag

Tests

  • Unskipped and passed the two failing tests mentioned in the issue
  • 3 new tests in effect.test translated from alien-signals (Passed)
  • 3 new tests in effectScope.test translated from alien-signals (Passed)

@littensy

Copy link
Copy Markdown
Owner

My only concern is that I intended for Charm to use first-in-first-out effect cleanup order since that seems more intuitive, and reversing this order might be a breaking change.

I want to take a closer look at this when I'm able to and maybe propose changes to match reactivity's FIFO effect cleanup.

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