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

Computed/ComputedPairs/other reactive graph objects should error on yield #15

Open
dphfox opened this issue Aug 20, 2021 · 3 comments
Open
Assignees
Labels
enhancement New feature or request ready to work on Enhancements/changes ready to be made

Comments

@dphfox
Copy link
Owner

dphfox commented Aug 20, 2021

Yielding is not allowed during reactive graph updates in order to ensure that the entire reactive tree is internally consistent. However, there's nothing currently stopping users from attempting to yield inside a computed callback.

Yielding like this can break a lot of things that assume no yielding occurs, for example the automatic dependency manager.

This isn't a blocking issue for initial beta release, but needs to be implemented before we move out of beta as it's an important error to catch.

@dphfox dphfox added the enhancement New feature or request label Aug 20, 2021
@dphfox dphfox added the ready to work on Enhancements/changes ready to be made label Sep 29, 2021
@nottirb
Copy link
Contributor

nottirb commented Dec 22, 2021

Was toying around with some stuff and came across some issues that might come up, should an Observer's update function be allowed to "yield". I say "yield" here because we currently call coroutine.wrap(callback)(), but I pose the thought that we should change this to dontYield(callback), along with #99.

My basis for this is that if you yield in an observer callback, there's no guarantee that state will be the same before and after the code that yields occurs. I think by default this should be changed to dontYield(callback), and if someone has a specific scenario where they need to yield (for whatever reason), they can just put coroutine.wrap() or something similar in their callback code.

WDYT?

@dphfox
Copy link
Owner Author

dphfox commented Dec 22, 2021

The yielding restriction only exists because of two reasons:

  • all state objects must update instantly so the state stored in the reactive graph is always internally consistent
  • any time dependencies are being captured, a yield may cause the automatic dependency manager to break

Observer does not store state, and does not interface with the automatic dependency manager. Therefore there's no good reason to block yielding here in my mind. It's meant to be an escape hatch for when you need to do something in response to a reactive graph change which you would otherwise not be able to express.

I don't think it should be Fusion's job to babysit code of that nature. Do you have a compelling concrete scenario where disallowing yielding for the reason you gave would be of substantial value to the developer?

@dphfox
Copy link
Owner Author

dphfox commented Jan 29, 2022

I'm deferring this to v0.3, since we're redoing how dependency management works there. This will change one of the assumptions upon which this issue depends upon; since there will no longer be an automatic dependency manager, and the flow of dependencies will be made explicit in user code, yielding will no longer break this.

This means that, as long as we can address having the reactive graph always be in a deterministic, internally consistent state, there may actually be merit to allowing for some of these objects to yield. Perhaps this may relate to #4?

@dphfox dphfox moved this to To Do in Fusion 0.3 Jan 29, 2022
@dphfox dphfox linked a pull request Feb 3, 2022 that will close this issue
@dphfox dphfox moved this from To Do to In Progress in Fusion 0.3 Aug 23, 2023
@dphfox dphfox self-assigned this Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready to work on Enhancements/changes ready to be made
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

2 participants