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

[fabiospampinato]

I think there's a bug here:

sinuous/packages/sinuous/observable/src/observable.js

Line 28 in ece2c65

tracking = undefined;

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]

There might be a similar bug here too:

sinuous/packages/sinuous/observable/src/observable.js

Line 30 in ece2c65

tracking = prevTracking;

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

[tsugabloom]

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]

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]

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]

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

@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

	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]

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