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

External source will rerun disposed inTransition computation function if there is a suspense context #2275

Open
rprend opened this issue Sep 6, 2024 · 1 comment
Labels
bug Something isn't working

Comments

@rprend
Copy link

rprend commented Sep 6, 2024

Describe the bug

We setup a simplified repro here: https://playground.solidjs.com/anonymous/c8c3f9c0-505f-43d2-a4ec-16bb8dbc9ed7. Setup solid with an external source. A memo returns a signal value. We call an inTransition trigger to simulate an external dependency being invalidated while we are in a transition. This will dispose the inTransition computation. After this, we start another transition, and update the signal which invalidates the memo. Since we are in a transition, we attempt to track the disposed autorun, throwing an error.

For this to repro, there needs to be a suspense context. startTransition() checks for the existence of a suspense before marking the transition as running.

tagging @milomg who wrote the repro case.

Your Example Website or App

https://playground.solidjs.com/anonymous/c8c3f9c0-505f-43d2-a4ec-16bb8dbc9ed7

Steps to Reproduce the Bug or Issue

Go to https://playground.solidjs.com/anonymous/c8c3f9c0-505f-43d2-a4ec-16bb8dbc9ed7
Run the playground example
Notice that we throw an error when we rerun a disposed transition

Expected behavior

I would expect one of two solutions. First, make inTransition not autodispose. Currently it disposes immediately after the transition promise resolves: const triggerInTransition = () => startTransition(trigger).then(() => inTransition.dispose()). Second, if it does autodispose, we call the external source factory with the transition trigger to replace the disposed one.

We work around this on our end by ignoring the dispose() calls for inTransition computations, but this is not ideal because it leaks memory.

Screenshots or Videos

No response

Platform

  • OS: macOS
  • Browser: Chrome Version 127.0.6533.120
  • Version: [e.g. 91.1]

Additional context

No response

@milomg
Copy link
Member

milomg commented Sep 6, 2024

I believe something like this should help (although ideally we'd also dispose when transitions that are created elsewhere are merged)

  if (ExternalSourceConfig && c.fn) {
    const [track, trigger] = createSignal<void>(undefined, { equals: false });
    const ordinary = ExternalSourceConfig.factory(c.fn, trigger);
    onCleanup(() => ordinary.dispose());
    const triggerInTransition: () => void = () =>
      startTransition(trigger).then(() => {
        inTransition.dispose();
        inTransition = undefined
      });
    let inTransition = undefined;
    c.fn = x => {
      track();
      return Transition && Transition.running ? (inTransition || inTransition = ExternalSourceConfig.factory(c.fn, triggerInTransition)).track(x) : ordinary.track(x);
    };
  }

@ryansolid ryansolid added the bug Something isn't working label Sep 23, 2024
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