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

Resetting the active root only if the currently disposed one is the active one #189

Open
fabiospampinato opened this issue Jan 27, 2022 · 7 comments
Labels
bug Something isn't working

Comments

@fabiospampinato
Copy link

I think there's a bug here:

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.

@fabiospampinato
Copy link
Author

There might be a similar bug here too:

We might be setting as active root one that got disposed in the child root.

@nettybun
Copy link
Contributor

I think it's working as expected... but let me know if am not understanding your concerns correctly:

The root for the fn's execution (1) is designed to be the only root. If the fn function decides to explicitly call dispose() (2) during its own execution, we'd want the remainder of it's execution to be rootless (undefined) rather than the previous root. Since the = undefined (3) is only ever executed during the fn call (since that's the only time dispose() (2) is defined), this works to make it rootless. When fn is finally done running, we set the root to the previous root (4) which is not the root that was disposed during fn, since it was saved at the start.

@fabiospampinato
Copy link
Author

fabiospampinato commented Jan 29, 2022

Since the = undefined (3) is only ever executed during the fn call

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.

@fabiospampinato
Copy link
Author

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 🤔

@luwes luwes added the bug Something isn't working label Jan 31, 2022
@luwes
Copy link
Owner

luwes commented Jan 31, 2022

@fabiospampinato do you have a use case where this is failing or a test?
I haven't run into this issue but would be nice to fix if this is breaking apps.

@fabiospampinato
Copy link
Author

fabiospampinato commented Jan 31, 2022

@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:

test('works from the body of its own computation', function(t) {
root(function(dispose) {
var c = 0,
d = o(0),
f = S(function() {
c++;
if (d()) dispose();
d();
});
t.equal(c, 1);
d(1);
t.equal(c, 2);
d(2);
t.equal(c, 2);
t.end();
});
});

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.

@luwes
Copy link
Owner

luwes commented Jan 31, 2022

thanks @fabiospampinato, I'll have a look at this soon.

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

No branches or pull requests

3 participants