fix: don't notify signals tracking AsyncSignal.future when transitioning to loading state by Yegair · Pull Request #434 · rodydavis/signals.dart

Yegair · 2025-12-09T10:44:41Z

Fixes #433

Fixes AsyncSignal.future to no longer notify when the async signal transitions into a loading state. The notification now happens when the async signal completes with data or error.
Adds AsyncSignal.completion for tracking dependencies across async gaps without getting notified about state changes to loading states.

zupat · 2025-12-09T11:26:21Z

packages/signals_core/lib/src/async/signal.dart

+
+  /// The future of the signal completer
+  Future<T> get completionFuture {
+    untracked(() {


This seems redundant as it was only accessed so it could be tracked.

I think it is necessary, to avoid creating a dependency from the caller to the signal.value, which is what causes the original problem

I am saying that it should be removed completely because value was not used or returned, it's original purpose was to register the caller as a dependent. Now that you're returning the internal signal, you can remove the untracked statement as it's no longer needed.

Oh I see. The reason for this is that I somehow have to "trigger" the async computation. Without this call, the future won't ever be resolved. This is related to how StreamSignal is implemented, which is the base for FutureSignal.

signals.dart/packages/signals_core/lib/src/async/stream.dart

Lines 350 to 357 in 5ac7164

@override

AsyncState<T> get value {

_cleanup ??= _stream.subscribe((src) {

reset();

execute(src);

});

return super.value;

}

However, I just did it, to get it working quickly. There might be a more elegant way to do this. Will take a closer look at this 👍

Oh, I didn't realize that .value had a side effect that needs to run to kick things off. This should probably be moved to the constructor but I admit that it is more efficient to not subscribe until the value is accessed.

This is related to how StreamSignal is implemented, which is the base for FutureSignal

Would it be better if FutureSignal did not extend StreamSignal? I have been thinking about making that change for v7

I generally like that there is a single implementation taking care of handling the async computation.

Given my current knowledge about the codebase I can't say whether separating FutureSignal from StreamSignal would make it easier or harder to implement changes/fixes such as this one.

If I had to make that decision, I'd look out for implementation details within StreamSignal that exist just to make FutureSignal work. If such code exists, then I'd say there is a good reason to no longer base FutureSignal on StreamSignal. However, if such code does not exist, then I think it makes more sense to keep it as it is.

Moved the call into StreamSignal, assuming that it is not needed for all other async signals. This should make it more obvious why it exists for future readers.

Yegair · 2025-12-10T11:01:51Z

Did a first implementation that passes all the existing tests.

Here is what I'm planning to do before submitting the PR for review:

~~Add more tests (have to figure out all the possible edge cases first)~~
~~Update commit messages~~
~~Update PR title & description~~

codecov · 2025-12-10T21:29:56Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.81%. Comparing base (14c0fa9) to head (f62c54b).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #434      +/-   ##
==========================================
+ Coverage   64.42%   64.81%   +0.38%     
==========================================
  Files          87       88       +1     
  Lines        2145     2194      +49     
==========================================
+ Hits         1382     1422      +40     
- Misses        763      772       +9

Files with missing lines	Coverage Δ
packages/signals_core/lib/src/async/computed.dart	`100.00% <ø> (ø)`
packages/signals_core/lib/src/async/future.dart	`100.00% <ø> (ø)`
packages/signals_core/lib/src/async/signal.dart	`100.00% <100.00%> (ø)`
packages/signals_core/lib/src/async/stream.dart	`100.00% <100.00%> (ø)`

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…ing to loading state

Yegair · 2025-12-10T21:42:51Z

packages/signals_core/lib/src/async/signal.dart

+  bool _initialized = false;
+  bool get isCompleted => _initialized && _completer.isCompleted;


The _initialized flag is needed to make an existing test for simple AsyncSignals work, in case they are created in a completed state.

signals.dart/packages/signals_core/test/async/signal_test.dart

Lines 16 to 21 in 5ac7164

test('isCompleted', () async {

final s = asyncSignal(AsyncState<int>.data(0));

expect(s.isCompleted, false);

s.setValue(1);

expect(s.isCompleted, true);

});

Didn't manage to figure out why an async signal is considered not completed after creating with data, so added this edge case in order to not break things.

Yep that sounds good!

Yegair · 2025-12-10T21:45:46Z

packages/signals_core/lib/src/async/signal.dart

+  static Future<T> _ignoreUncaughtErrors<T>(Future<T> future) {
+    // Makes sure the future never reports an uncaught error to
+    // the current zone. Seems to be necessary to avoid uncaught
+    // errors to be reported when async signals are being used
+    // synchronously, i.e. when their .future is not used.
+    future.ignore();
+    return future;
+  }


Here I'm really not sure if this is a good idea, but IMHO we have no other choice. Otherwise users might see a lot of uncaught errors even when they are handled synchronously through AsyncError

zupat reviewed Dec 9, 2025

View reviewed changes

Yegair changed the title ~~wip: adds experimental .completionFuture and .completion to AsyncSignal~~ fix: don't notify signals tracking AsyncSignal.future when transitioning to loading state Dec 10, 2025

fix: don't notify signals tracking AsyncSignal.future when transition…

a067d1b

…ing to loading state

Yegair force-pushed the fix/433-future-signal-executed-too-often branch from f62c54b to a067d1b Compare December 10, 2025 21:36

Yegair marked this pull request as ready for review December 10, 2025 21:37

Yegair commented Dec 10, 2025

View reviewed changes

	@override
	AsyncState<T> get value {
	_cleanup ??= _stream.subscribe((src) {
	reset();
	execute(src);
	});
	return super.value;
	}

		bool _initialized = false;
		bool get isCompleted => _initialized && _completer.isCompleted;

	test('isCompleted', () async {
	final s = asyncSignal(AsyncState<int>.data(0));
	expect(s.isCompleted, false);
	s.setValue(1);
	expect(s.isCompleted, true);
	});

Conversation

Yegair commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zupat Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yegair commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 10, 2025

Codecov Report

Uh oh!

Yegair Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Yegair commented Dec 9, 2025 •

edited

Loading

zupat Dec 9, 2025 •

edited

Loading

Yegair commented Dec 10, 2025 •

edited

Loading

Yegair Dec 10, 2025 •

edited

Loading