Breaking change in runWithOwner

[Exelord]

Describe the bug

In this commit 54f3068 error handling has been added which caused a change in the interface, breaking backwards compatibility, and causing users to fail.

Your Example Website or App

54f3068

Steps to Reproduce the Bug or Issue

runWithOwner(owner, () => throw new Error("error")); // may or may not throw an error

Expected behavior

runWithOwner should stay as a primitive to run code with different owner. Currently it is doing much more than that, like handling errors and managing Listener. This make it impossible to use it for the purpose described in the docs.

I think we should create another helper, and leave runWithOwner as a primitive to allow handle advanced use cases.

Screenshots or Videos

No response

Platform

• OS: [e.g. macOS, Windows, Linux]
• Browser: [e.g. Chrome, Safari, Firefox]
• Version: [e.g. 91.1]

Additional context

No response

[thetarnav]

What have you been doing with runWithOwner before that is not possible now?
It not owning errors was causing batch to break the ability to throw errors to the ErrorBoundry. #1452
And Listener not being reset meant that you could run a callback with different Owner and Listener. #1495
It still does exactly what it's meant to do. It's just less buggy.

[Exelord]

I was using it to execute a code in scope of other owner. However now, it is also changing the Listener as well us not throwing an error is case there is an error boundary - which is not what I expected from runWithOwner. It is doing much more than that.

The breaking change does not allow anymore to relay on the result as in case of error it might return undefined. There is no way now to distinguish between an error and actually returned undefined value.

const result = runWithOwner(owner, () => {
  return init()
})

result // undefined - is it an error or a return from init?

I would suggest reverting it to a primitive, and build a higher level helper to support those cases to bring back the original behaviour.

[thetarnav]

Isn't this the same behavior as any other reactive primitive? instead of throwing and freezing the app, errors will be caught by the error boundary.
If you want something to throw, you shouldn't do that when running something with owner. I think. But could you schedule a microtask?

const result = runWithOwner(owner, () => {
  return { v: init() }
})

result // undefined - error thrown, { v: T } - ran without errors

I consider the previous behavior a bug, but you may have a different usecase for these.
What exactly are you doing? Why the need to catch errors, but also run the code with an owner? Never needed this so I'm curious.
Maybe there should be separate primitives to choose depending on who is supposed to catch errors 🤷‍♂️

[Exelord]

I don't see runWithOwner as a control flow function. For me it has nothing to do with error boundary the same as Owner. The function which is calling runWithOwner should decide how to handle its error. It is a very low level primitive that was introduced to not expose setOwner function, to maintain the integrity in case the owner was not set back.

I think we could go even further now and control Listener in the same way:

export function runWith<T>(
  { listener, owner }: { owner?: typeof Owner; listener?: typeof Listener },
  fn: () => T
): T {
  const prevListener = Listener;
  const prevOwner = Owner;

  if (owner) Owner = owner;
  if (listener) Listener = listener;

  try {
    return runUpdates(fn, true)!;
  } finally {
    Owner = prevOwner;
    Listener = prevListener;
  }
}

My use cases are in global state management as well as in async world, where I want to be able to run a chunk of code under some specific place in reactivity tree. Do exactly what .bind(this) is doing to functions. It would be weird if bind would be handling errors wouldn't it? I am aware of the purpose of Owner and Listener, and I want to be in control of them.

Your suggestion is a huge hack :/ and it's still not fixing the breaking change. Instead we could just use try catch to handle the error and expose API if needed, or build a function that will be like:

export function wrapWithOwner(o: typeof Owner, fn: () => T): void {
  try {
    runWithOwner(o, fn)
  } catch (error) {
    handleError(error)
  }
}

This way we would avoid a breaking change, have an access to a powerful and self-healing primitive, as well as be able to handle errors without exposing internals.

[Exelord]

I hope this example could help understand my reasoning:

function a() { ... }

async function b(owner, fn) {
  const v = runWithOwner(owner, fn)
  await sendThisToServer(v)
}

// somewhere in the app
b(owner, () => "hello")

The b function can be executed in any context and place. I want to be in control of how its error is handling. I wouldn't expect that runWithOwner will intercept the exception and throw it in some error boundary I am not aware of. As the caller of b I should be responsible for what to do with that error.

Maybe there should be some API allowing to throw an error in specific boundary:

export function handleErrorWithOwner(owner: typeof Owner, error: any) {
  runWithOwner(owner, () => handleError(error));
}

[ryansolid]

Hmm.. I think the previous Listener behavior was wrong. There is no way that setting the owner should have let the Listener through. It was an oversight and never intended to be that way. I think runWithListener might be just too low level and cause way more issues than it is worth. It would remove the need for createReaction I suppose, but attacking the API this way in general is sort of messy.

Error is a more interesting discussion. The problem is that runWithOwner is resuming a reactive scope. I think the expectation is very much that it would play into the error boundary. Like if not to insert it in the reactive context why do it? Batch very much was swallowing these errors anyway from the perspective runWIthOwner shouldn't catch, and if batch were ever to be removed from Solid's API surface I'd expect the owner to catch it.

runWithOwner is the start of the boundary so my gut would be if I was creating b and really wanted to take it on myself, that I'd have to make that decision

async function b(owner, fn) {
  let error;
  const v = runWithOwner(owner, () => {
    try {
      return fn();
    } catch (err) {
      error = err;
    }
  })
  if (error) throw error;
  await sendThisToServer(v)
}

This isn't pretty but I am having a hard time picturing where this would not be the expectation. And adding API surface for this feels like taking complexity to an unwanted place. Opting into error boundaries seems unexpected as you already told it to run under this context.

That being all said I can sympathize this is breaking behavior. I very much viewed it as an oversight on my part. A bug. We could revert some things and make this into 1.7 release if that makes more sense. I don't think this is a wait until 2.0 sort of thing. It was very much broken before these "fixes".

I wouldn't mind more opinions on this. But I suspect it is super niche.

[LiQuidProQuo]

related discussion:
runWithOwner / runWithListener support tracking asynchronously #1494

[Exelord]

I think our confusion here is about the purpose of runWithOwner. It seams like it evolved to a specific version of runUpdates that is meant to handle all reactivity cases.

While we were designing it in #323 it was meant to be a low level primitive - the same level as runWithListener. As you are probably aware runWithListener actually exist in our API right now. However it's limited and does not allow to specify custom Listener - It's called untrack. And it acts as a shorthand for runWithListener(null, fn). It's purpose is on the same level as runWithOwner however one is more capable than the other.

If I am currently able to do:

untrack(() => runWithOwner(owner, () => "hello"))
// equal to 
runWithListener(null, () => runWithOwner(owner, () => "hello"))

I don't see a reason why I shouldn't be able to specify my own listener if I am able to get it using getListener:

runWithListener(listener, () => runWithOwner(owner, () => "hello"))

---

Going even further, now I have a case where I need to run a chunk of code without an owner - which is currently not supported by runWithOwner. The same need has createResource which in its first execution of fetcher, it runs it with an owner, while during refetch it doesn't. This can create issues if someone used eg createEffect inside of a fetcher. The fetcher should always be run without an owner or with an owner that is disposed at the end of execution.

---

These low level functions are important to keep Solid open for advanced integrations. While we are not able to predict all use cases, my presence here proves we did miss a case. The open API for reactivity is in my opinion a huge strength of Solid, which is not limiting it to its built-in utilities functions, and allows for plenty of use cases to be handled.

The current design of runWithOwner shifted a lot during last months. While I understand the need and use case in batch, I don't think it should be built around it, and we should't assume that we will always want to run runWithOwner with untrack/runWithListener(null, fn) combined. That is limiting integrations possibilities.

Instead I think we should at least preserve runWithOwner as a primitive, working exactly like untrack. Ideally, to require the user to think about the reactive context, we could introduce a combined function runWith which would require to specify owner and listener explicitly:

export function runWith<T>(
  { listener, owner }: { owner: typeof Owner; listener: typeof Listener },
  fn: () => T
): T {
  const prevListener = Listener;
  const prevOwner = Owner;

  if (owner) Owner = owner;
  if (listener) Listener = listener;

  try {
    return fn();
  } finally {
    Owner = prevOwner;
    Listener = prevListener;
  }
}

To handle the current use case for batch, we could introduce:

export function runUpdatesWithOwner(owner: typeof Owner, fn: () => unknown): void {
  runWith({ owner, listener: null }, () => {
    try {
      return fn();
    } catch (error) {
      handleError(error);
    }
  }
}

That would work as another utility and handle one of the use cases. Please mind that runUpdatesWithOwner would not return anything to preserve consistency with error handling.

---

Furthermore, I think we have a bug in runUpdates as its return type is T | undefined meaning we do handle error there but only in case there are no pending updates. Effected methods like createRoot and batch works only cuz we explicitly forced typing.

It the example pointed by Damian:

batch(() => runWithOwner(owner, () => {
  throw new Error("Batch and Throw in owner");
}))

we are running runWithOwner inside of batch. And runWithOwner is running runUpdates inside + doing errors handling. However the batch itself is wrapper around runWithOwner. That leads me to think that runWithOwner shouldn't be calling runUpdates at all, and the error should be handled on batch -> runUpdates level by changing it to:

function runUpdates<T>(fn: () => T, init: boolean) {
  if (Updates) {
    try {
      return fn();
    } catch (error) {
      handleError(error);
    }
  }
 ...
}

For consistency we could even stop returning value there, as we are not able to guarantee the result.

Please correct me if Im wrong in my thinking process.

I think we used runWithOwner as a hotfix for underlaying issues, changing its purpose and function. If the project is following SemVer, we should definitely revert the changes back, as a change in interface is braking integrations' functionality.

I would be happy to talk and work on it more, as it looks like the issue is more complex than initially assumed.

[thetarnav]

I feel like a lot of these "breaking changes" happen because some behavior is assumed, but not actually unit-tested here, so it's easy to override it.
Is it correct to assume that the error thrown here will be caught by an error boundary?
If so, should I add tests for it?

batch(() => runWithOwner(owner, () => {
  throw new Error("Batch and Throw in owner");
}))

In my mind, runWithOwner is a way to run an effect asynchronously and so it should behave the same way as if this was a createEffect callback. Minus the tracking because it's not runWithListener, which would probably behave the same way if added, and not like untrack which doesn't care about updates because it doesn't have to. When you are using untrack you are already in some owner context, which will handle updates and errors. runWithOwner or runWithListener cannot be as simple as untrack, at least not in public and documented API.
I would love to have access to lower-level primitives as well, but exposing them must be scary from Solid's point of view.
And runWithOwner is not that anymore. Neither getOwner nor getListener is tbh.
So the API should probably be reworked for 2.0 so that users don't ever have to manage the Owner object themselves.
It could be replaced with something like this:

const runEffect = createCallback() // replaces runWithOwner

<button onClick={() => {
  runEffect(() => {
    throw "can throw and ErrorBoundary will catch it"
    
    createEffect(() => {
      console.log("can create effect that will be disposed with parent owner")
    })
  })
}} />

// replaces getOwner and getListener
hasOwner() // => boolean
hasListener() // => boolean

if (!hasOwner()) {
  console.warn("this should be called under a reactive owner!!")
}

[ryansolid]

I think we need to trace back to when runWithOwner became runUpdates that's probably where the split of mindset happened. That was before this more recent change. The recent change was sort of inevitable given the rest. But it is sort of the problem. I want to stress that batch has to be treated as optional. I'm pretty sure that was how we got here.

Like someone tried to resume a context and something wasn't working. I see that when I changed Jan 3rd 2022. I added a test to make sure created effects and cleanups ran. I'm not sure why though. I was working on streaming at the time and fixing up some concurrency bugs. It must have been for errors. I probably put it in runInUpdates to avoid having to write customer error handling stuff in there. But seems were are here anyway.

Which brings me to.

@Exelord I'm less clear still about the actual use cases. When you proposed it I was definitely thinking more like Damien's examples which is why I compared it to Context. But immediately saw how this makes it so easy to break things in unpredictable ways. I needed the tool internally so I thought it was reasonable to expose it.

getListener really could have been isTracking from my perspective but I was a bit like why not. These sort of things were probably not for the best.

@thetarnav that's a cool API idea. I'd love not to expose Owner although it also is used with createRoot as well to inject owning context. Maybe that's the trick though. Have this singular way of doing it. Even with createRoot.

[Exelord]

Yes, I agree. The issue seams to not be related to latest changes. As you mentioned they are consequences of using runUpdates inside.

I see, then I guess our intentions diverged. I have never seen runWithOwner as a function to run "effect" externally. However, it might be exactly what we need.

Here is my use case demo: https://playground.solidjs.com/anonymous/b782b478-0803-4595-b30a-211fbd4c2476

What Im trying to achieve is:

• I need to be sure that error stops further code execution
• the error needs to be first handled by a local onError, if not present propagated further
• I need to be sure the correct context is used
• in case of error, local cleanups hooks should be run
• parent should also dispose local cleanups

So far, I am able to implement it all using custom function:

function runInOwner<T>(owner: Owner, fn: () => T) {
  let error;
  let hasErrored = false;

  const result = runWithOwner(owner, () => {
    let disposer: () => void;
    onCleanup(() => disposer?.());

    return createRoot((dispose) => {
      disposer = dispose;
      try {
        return fn();
      } catch (e) {
        dispose();
        hasErrored = true;
        error = e;
        return;
      }
    }, owner);
  })!;

  if (hasErrored) throw error; // We need to throw error here in order to stop further execution of parent function

  return result;
}

I would love to simplify it into:

function runInOwner<T>(owner: typeof Owner, fn: () => T): T {
  return runWithOwner(owner, () => {
    let disposer: () => void;
    onCleanup(() => disposer?.());

    return createRoot((dispose) => {
      disposer = dispose;
      try {
        return fn();
      } catch (e) {
        dispose();
        throw error;
      }
    }, owner);
  })!;
}

and maybe eventually built it into solid, removing a need for runWithOwner (current implementation).

---

I just thought, If we could support custom owner for onCleanup we could land with sth like:

export function runWithOwner<T>(o: Owner, fn: RootFunction<T>): T {
  return createRoot(dispose => {
    try {
      const result = fn(dispose);
      onCleanup(dispose, o);
      return result;
    } catch (error) {
      dispose();
      throw error;
    }
  }, o);
}

eventually even resigning from runWithOwner API

[Exelord]

It looks like I have found an elegant solution to my issue:

export function runInSubRoot<T>(
  fn: RootFunction<T>,
  owner?: typeof Owner,
  cleanup = false
): T {
  return createSubRoot((dispose) => {
    onError((error) => {
      dispose();
      throw error;
    });

    const result = fn(dispose);
    if (cleanup) dispose();
    return result;
  }, owner);
}

Now I am more convinced that we don't need runWithOwner to be exposed publicly, if we will add custom owner support for onCleanup, onError and useContext as this is the only use case for it now.

@ryansolid what do you think about the idea?

[ryansolid]

Not sure.. We've been playing with the idea that roots should have no impact on owner. Like they just keep their current owner, and then runWithOwner is the only way to change owner.

Then I thought runWithOwner === untrack + owner argument. Which aligns with onCleanup and onError. But you are right with createRoot, onCleanup, and onError more or less could mean not having it.

[Exelord]

Isn't the purpose of root to create a new owner? And the detachedOwner is basically an origin?

If other APIs would support optionally a custom owner:
-onCleanup(() => {}, owner)
-onError(() => {}, owner)
-useContext(() => {}, owner)

then there will be never a case to use runWithOwner. It is much more safer to use createRoot as we are able to isolate the reactive context, being sure it won't influence original context. Sure, performance wise, it might be a little worse as each time we create an owner, however it is much more safer way to interact between boundaries and it gives control over cleaning up in case of error.

I think runWithOwner as a util could stay but its mechanic need to change and become one time runner. Otherwise its purpose will be duplicating createRoot.

So funny how I went from "I need original runWithOwner" to "we should remove it" 😅 I have learned so much from this exploration, especially about being "branch" oriented instead of "portal"

[ryansolid]

I think this belongs as a discussion rather than an open issue as to inform 2.0 design.