Run dispose before effect when keys change (#335)

[jezsung]

Summary

See #335

This PR changes the execution order of useEffect dispose and effect callbacks when keys change. Previously, the new effect ran before the old effect's dispose callback. Now, dispose runs first, matching React's useEffect behavior.

Problem

When useEffect keys changed, the execution order was:

1. New effect runs
2. Old effect's dispose runs

This could cause issues when the dispose callback needs to clean up resources before the new effect initializes them.

Solution

Changed the execution order so that when keys change:

1. Old effect's dispose runs first
2. New effect initializes second

Tests

Added regression tests to verify the execution order:

• dispose runs before effect when keys change
• dispose runs before effect on every build when keys are not provided
• dispose and effect run in group for each hook in declaration order

Added tests to ensure errors in dispose are handled properly:

• errors in dispose are handled when keys change
• errors in dispose are handled on unmount

Updated existing tests to match the corrected behavior:

• useEffect disposer called whenever callback called (use_effect_test.dart)
• hooks are disposed in reverse order when their keys changes (hook_widget_test.dart)
• keys recreate hookstate (hook_widget_test.dart)

Note on React Compatibility

This PR still does not fully match React's useEffect execution order due to fundamental architectural differences between Flutter and React:

1. Multiple hooks in the same widget: React batches all disposes before all effects (dispose1 → dispose2 → effect1 → effect2). This PR maintains synchronous per-hook execution (dispose1 → effect1 → dispose2 → effect2) to preserve backward compatibility with existing code that relies on effects running immediately during build.

2. Parent-child ordering: React runs child effects before parent effects (bottom-up) and parent cleanups before child cleanups on unmount (top-down). Achieving this in Flutter would require deferring effects to a post-frame callback, which would be a breaking change to the synchronous execution model.

Whether flutter_hooks should fully match these React behaviors is debatable. Doing so would introduce significant breaking changes, and it's unclear whether React's execution model is inherently better than Flutter hooks' model.

Summary by CodeRabbit

• Bug Fixes

  • Hooks now dispose immediately when replaced by a different hook type, and disposal runs before the next effect when dependencies change, improving lifecycle ordering and cleanup reliability.

• Tests

  • Added and updated tests covering disposal ordering, keyed/unkeyed effect sequencing, per-hook ordering, and error propagation from disposers (including unmount).

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Hook disposal timing was changed: when a hook is replaced by a different type, its state is disposed immediately (with errors reported), then a new state is created. Tests were updated and expanded to reflect and validate the new dispose-before-recreate semantics.

Changes

Cohort / File(s)	Summary
Core implementation packages/flutter_hooks/lib/src/framework.dart	HookElement._use now disposes the mismatched existing hook state immediately (try/catch + FlutterError on failure) and then creates the new hook state; removes deferred _needDispose queuing.
Hook widget tests packages/flutter_hooks/test/hook_widget_test.dart	Adjusted verifyInOrder expectations to expect an immediate dispose() before recreation; removed expectations for the previously queued trailing dispose.
useEffect lifecycle tests packages/flutter_hooks/test/use_effect_test.dart	Reordered expectations and added tests covering: dispose-before-effect when deps change, dispose errors propagation when deps change and on unmount, keyed vs unkeyed behavior, and per-hook ordering across multiple useEffect calls.
Manifest pubspec.yaml	No declaration/API changes; manifest touched (unchanged public exports).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐇 I nibble at code where hooks once queued,

I tuck old states away — then bid them adieu.
Dispose comes first, then springs the new part,
Fresh hooks hop in with a lighter heart. ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The pull request title directly summarizes the main change: modifying useEffect dispose/effect execution order when keys change.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@packages/flutter_hooks/lib/src/framework.dart`:
- Around line 479-482: The inline call to _currentHookState!.value.dispose()
lacks error handling and can abort creation of the new hook state; wrap that
dispose() call in a try-catch and, if an exception occurs, report it using
FlutterError.reportError with a FlutterErrorDetails (include exception, stack,
library/context like 'flutter_hooks: dispose in update hook', and a helpful
ErrorDescription) before continuing to set _currentHookState!.value =
_createHookState<R>(hook), matching the pattern used in unmount() to ensure the
new hook state is still created even if dispose() throws.