solid icon indicating copy to clipboard operation
solid copied to clipboard

The order of the effects changed in SolidJS 1.5

Open juanrgm opened this issue 2 years ago • 9 comments

Describe the bug

Hi all.

I am facing a problem after upgrading to SolidJS 1.5 (https://github.com/swordev/suid/issues/92) and I don't know if this behavior change is documented.

Your Example Website or App

https://playground.solidjs.com/?version=1.5.3#NobwRAdghgtgpmAXGGUCWEwBowBcCeADgsrgM4Ae2YZA9gK4BOAxiWGjIbY7gAQi9GcCABM4jXgF9eAM0a0YvADo1aAGzQiAtACsyAegDucAEYqA3EogcuPfr2ZCouOAFEZMuM1xYHTlwDKaADm0Gq+tBAAsgwQfNJyCsqqGtp6FlZWcBS2fGIyUPRqfDL0EN5okbwAwrEujAAUAJT8VrwOkWR8wDB1cCK+ZHC4MWUuIgC6vAC8fnDOcEGhUGoNBWpDTZYQbbyRo3ENzTMAfK0Q7e1DI30iDbiM9HBbu5IvOxdzC+6e3kct0zOIF27WYnXUcAAdGpaMEGipemN+ipfIi4v1mu92m9trshLgmBcADwiNAANxOIDR42akNwtACDwwcKakiJ+lJFO2kkyECEonE-1OvCJtSREn0J18IlozHo8DikOCw1cajgCtwACF8ABJO4qKCEQgqJoAQneYEkEyAA

Steps to Reproduce the Bug or Issue

Code

import { render } from "solid-js/web";
import { createEffect, createSignal, onMount } from "solid-js";

export default function Counter() {
  const [mounted, setMounted] = createSignal(false);

  onMount(() => {
    setMounted(true);
  });

  createEffect(() => {
    console.log("mounted", mounted());
  });

  return <div>{mounted().toString()}</div>;
}

render(() => <Counter />, document.getElementById("app")!);

mounted true

mounted false mounted true

Expected behavior

mounted false mounted true

Other example

Screenshots or Videos

No response

Platform

  • OS: Windows
  • Browser: Chrome
  • Version: 104

Additional context

No response

juanrgm avatar Sep 01 '22 19:09 juanrgm

Yes this was the batching change we advertised in the release. https://github.com/solidjs/solid/releases/tag/v1.5.0

Which is the big change here. It's not that the effects run in a different order, but that changes are applied immediately. In general using effects to synchronize state isn't great and there is a tradeoff here. The new behavior removes something that kept on tripping up developers as Solid in some places would update signals immediately and others not. And it wasn't something they opted into.

I honestly think an argument for the old behavior from a design perspective makes a lot of sense and I made that argument for years. But it did really complicate everything from a learning perspective as well.

The simple solution is to queue your setter on the next microtask to get close to the previous behavior back. And this fix should be backwards compatible.

ryansolid avatar Sep 01 '22 20:09 ryansolid

I understand the background but, for me and now (no offense), SolidJS does magic. I will need to relearn SolidJS.

juanrgm avatar Sep 01 '22 22:09 juanrgm

Is it like if createEffect is before onMount, the result is different?

If so it could be a problem I guess.

yw662 avatar Sep 01 '22 22:09 yw662

Is it like if createEffect is before onMount, the result is different?

If so it could be a problem I guess.

Yeah this is the one part if there was one that I feel the least happy about. However it does solve a bunch of other issues. And it's is visibly obvious this can be the case.

I understand the background but, for me and now (no offense), SolidJS does magic. I will need to relearn SolidJS.

@juanrgm I can respect that. Because it does change the expectations. Interestingly enough this was always under the hood. The changes to batch were actually just deleting a bunch of stuff. Trying to force the old behavior was a bit unnatural but something I thought necessary. But it was constantly a source of issues. This wasn't an easy decision and we spent several months debating this in the discord. Probably should have engaged the RFC process earlier (like we are with Solid 2.0). But this seems to be the behavior that aligns the most with other people's expectations. I say debate but it was more like myself alone defending the old behavior and everyone else wanting this. After exploring many different options, writing an article about it: https://dev.to/this-is-learning/the-cost-of-consistency-in-ui-frameworks-4agi and testing a different alternative we didn't ultimately adopt here we are.

We need more docs I admit but the funny thing about it was for the most part we only needed to do deletion in the docs as well and new Solid developers won't be tripped up. This was a huge simplification. But I empathize with those comfortable with the previous behavior.

ryansolid avatar Sep 01 '22 22:09 ryansolid

FWIW I just wanted to mention that I learned that component level onCleanup is always called last, so in opposite to onMount it brings its own priority. onMount is just a short form for createEffect with untrack, you can have multiple onMounts being called before, after or between the effects and their cleanup functions will execute in the same order. I think there is some good logic behind it, but maybe the docs could reflect on that a little bit better, especially that component level onCleanup is not the perfectly symmetrical counterpart to onMount.

From a conceptual point of view, I think the 1.4.3 behaviour was less logical, executing onMount always first would be logical as well, but I could see situations where one wants to call it after some effect.

fweth avatar Sep 04 '22 20:09 fweth

One thing that we recommend everyone do is avoid setting state from most computations/hooks/effects/memos and ideally restructure your code to derive the state with a memo or at least make it explicit in your code that you expect the setter to happen next frame with requestAnimationFrame or requestIdeCallback. This may not be possible in all cases, and if you can't seem to figure out how to restructure your code, I'd love to see a minimal example to figure out what useful primitives solid core can provide or to help restructure your code.

milomg avatar Sep 05 '22 02:09 milomg

The original code was written in ReactJS and it was ported automatically to SolidJS with @suid/codemod.

Prior to SolidJS 1.5 the behavior was compatible with ReactJS, now I don't know if I can automate it.

I don't know if there are other similar cases derived from this change.

juanrgm avatar Sep 05 '22 19:09 juanrgm

Maybe find all onMount and wrap the inner function with raf/timeout.

yw662 avatar Sep 05 '22 19:09 yw662

It comes down to the setSignal calls from onMount or createEffect. onMount it is pretty safe to wrap the set calls in a queueMicrotask but effects are a bit trickier perhaps.

ryansolid avatar Sep 06 '22 03:09 ryansolid