createRoot creates the identical root on different calls if Function.prototype.length is 0 on the callback function - leads to roots being unexpectedly disposed when calling dispose() on any of them

[danieltroger]

Describe the bug

There's a bug in solid-js where, if the callback function passed to createRoot has .length that equals 0, always the same "owner scope" will be used, regardless of how often you call createRoot.

That is because this line:

solid/packages/solid/src/reactive/signal.ts

Line 111 in 7588777

? UNOWNED

says UNOWNED and not { ...UNOWNED }. So a new owner object isn't created, instead a reference to the previous one is used.

I'm also really skeptical of using Function.prototype.length like you're doing here:

solid/packages/solid/src/reactive/signal.ts

Line 110 in 7588777

fn.length === 0 && !"_SOLID_DEV_"

I don't understand the purpose but it doesn't work with arguments or rest parameters and I think it would be better to not use it.

You might ask why I don't just provide an argument as workaround, the reason is that I'm using a function to wrap my functions similar to this: https://playground.solidjs.com/?hash=1522254560&version=1.1.1

Your Example Website or App

https://playground.solidjs.com/?hash=1865669820&version=1.1.1

Steps to Reproduce the Bug or Issue

1. Go to https://playground.solidjs.com/?hash=1865669820&version=1.1.1
2. Click on a button

Expected behavior

Only the button that is clicked is removed.

Disposing one root/context/owner scope only calls onCleanup in the things that are tracked to it and not to other ones.

Current behavior

Both buttons are removed when clicking either

Platform

• OS: macOS 12.2 (21D49)
• Browser: Firefox
• Version: 101.0b9 (64-bit)

Additional context

I'm building parts of a website using solidjs. Additionally to these parts I'm also building a modal using solidjs.
These two parts run in two separate createRoots/"owner scopes" as per the documentation.
The idea is to dispose everything of the modal when it is closed but still keep the other part around until it should be disposed.

I have a ResizeObserver on the first part (which also is a springboard for the modal) which communicates values to the modal. I noticed that after closing the modal and opening it again there were no values from the ResizeObserver anymore (I disconnect it onCleanup).
Now much debugging later I have figured out what I've detailed in this issue report

[ryansolid]

Hmm.. this is intentional if I understand your situation. The idea is that except for top-level roots that will never be disposed you should be getting a dispose function otherwise there is no point in making a new root. I realize the function arity can be gamed by using args or default values etc, but generally speaking, this is a function with a single argument you use or don't. Why even bother? Probably doesn't matter much anymore but when using Unowned we can make some assumptions that lower memory usage and bookkeeping overhead. A small performance thing. Better than creating a top-level dispose that never gets called. Also allows GC to work on its own easier as it doesn't keep a lasting reference to child computations.

Of course, this wrapper case is such a case where this falls apart. So I agree relying on function arity is generally not good API unless it isn't achievable in another way. Which this qualifies as. Mind you I'm not sure it is important enough. It does make a measurable difference in reactive benchmarks. So this seems like a reasonable part of growing pains. I suppose the other option is to pass an additional argument, but that loses a bit of the just works part of this. Like the default being don't worry about disposal.

So tricky because the current API does exactly what is intended outside of this more edge case and there isn't really a suitable replacement. Besides just getting rid of this because it isn't that important (especially since render automatically opts into disposal). So can't change this without it being breaking so this is a 2.0 level consideration. I don't see a quick fix for now.

[danieltroger]

It is intentional that disposing one root also disposes the others? In that case maybe it would be good to state that in the documentation since I got the impression that every root is somewhat self-contained.

Thank you for explaining, I think it makes a lot of sense to have a "global" root that's used for roots that never get disposed.

Would you be open to make the dispose function a NOOP though if you don't think that it will be used (fn.length === 0) ? Maybe even print a helpful message on dev builds that you're using a hack to detect if people use the dispose function and they can't call dispose functions obtained by arguments or rest syntax.
I think it's better that a root isn't disposed and GCd rather than people getting bugs in their applications. For me it took around ~3h to debug and report and I would very much have appreciated a warning so that I can just work around it and move on with what I'm trying to accomplish.

[ryansolid]

Yeah for sure. And simply call it without dispose in prod. Sorry I feel like I used to do that. I don't know why or when I changed that. The random disposal is no good, it just shouldn't be disposable.