-
Notifications
You must be signed in to change notification settings - Fork 34
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
Resetting the active root only if the currently disposed one is the active one #189
Comments
There might be a similar bug here too:
We might be setting as active root one that got disposed in the child root. |
I think it's working as expected... but let me know if am not understanding your concerns correctly: The root for the |
That's not necessarily true though, I can create a root (A), create another root inside that (B), pass the disposer for B to A, call the disposer from A, after B has finished executing, in which case now A becomes rootless, which is not what we want, we wanted B to become rootless. I think there should be a little boolean that actually keeps track of whether we are inside the function or not, which would fix the problem of passing the disposer up the hierarchy, but there's a symmetric problem with passing the disposer down the hierarchy too, maybe we are calling the disposer from inside the function, but we are temporarily inside another root. |
I think a quick acceptable workaround for these issues would be throwing is the "dispose" function for a root is called while the current root is a different one, at least that would make things more understandable. Fixing this properly sounds a little convoluted 🤔 |
@fabiospampinato do you have a use case where this is failing or a test? |
@luwes I don't really have a practical use case for this yet, I'm currently kind of reimplementing sinuous' observable (great work by the way, I'm learning a lot from reading your code!) and while trying to fully understand the code I stumbled on that issue. If I make it throw an error in my implementation if the current "tracking" object is not the one being disposed the following test fails: sinuous/packages/sinuous/observable/test/dispose.js Lines 33 to 53 in c6d37a0
If the library doesn't throw the test kind of still works correctly, but I think if another observable is subscribed to after this line then that subscription shouldn't get registered with the enclosing computation, which would be an instance of the bug in question. |
thanks @fabiospampinato, I'll have a look at this soon. |
I think there's a bug here:
sinuous/packages/sinuous/observable/src/observable.js
Line 28 in ece2c65
Shouldn't it reset the current root only if the one that got disposed is the active one? Otherwise this would break with nested roots, I think.
The text was updated successfully, but these errors were encountered: