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

Bugfix: Accessing AsyncComputed status property should trigger computed function #88

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

aomarks
Copy link
Contributor

@aomarks aomarks commented Dec 10, 2024

Same as value, error, and complete already do. Otherwise, only reading status isn't sufficient for a dependency to get tracked.

@NullVoxPopuli NullVoxPopuli added the bug Something isn't working label Dec 10, 2024
@NullVoxPopuli NullVoxPopuli merged commit 0a6d3e0 into proposal-signals:main Dec 10, 2024
6 checks passed
@github-actions github-actions bot mentioned this pull request Dec 10, 2024
@justinfagnani
Copy link
Contributor

Oh, this behavior was somewhat intentional. I think I just copied the comment that mentioned that status ran the function incorrectly.

The idea is that while value and error don't really make sense to access if the function has never run, you may want to access status to see if the task has be initiated. Otherwise the initial state is very hard to ever observe.

I think we should keep an eye on this and see if anyone wants a non-triggering status getter. Maybe there's a separate getter for that.

@justinfagnani
Copy link
Contributor

The class comment should be updated too, since it doesn't mention status:

 * Compute functions are run when the `value`, `error`, or `complete` properties
 * are read, or when `get()` or `run()` are called, and are re-run when any
 * signals that they read change.

@aomarks
Copy link
Contributor Author

aomarks commented Dec 11, 2024

The idea is that while value and error don't really make sense to access if the function has never run, you may want to access status to see if the task has be initiated. Otherwise the initial state is very hard to ever observe.

Ah, I see. I found it surprising that accessing complete would trigger a run, but status didn't, so I figured this was an oversight. How about peekStatus for the side-effect free version?

aomarks added a commit to breadboard-ai/breadboard that referenced this pull request Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants