ryansolid / dom-expressions

A Fine-Grained Runtime for Performant DOM Rendering
MIT License
850 stars 122 forks source link

Fix: hydration mismatch of nestedDynamic #313

Closed birkskyum closed 4 months ago

birkskyum commented 4 months ago

Closes https://github.com/solidjs/solid/issues/2100#issuecomment-2002474213

[!WARNING]

Potential for side effects Unfinished as some dynamic tests fail

I made two small adjustments to utils.js and template.js, that will do as I outlined in the issue ticket linked above

For each fragment test code.js file I added:

const nestedDynamic = (
  <>{state.prop ? state.prop ? <div>First</div> : <></> : <></>}</>
);

And then i updated the snapshots by deleting them all (find . -name \*output.js -type f -delete) and running the tests.

It's still a bit unclear if the altered tests that were naïvely updated (i.e. here) were changed for the better or worse. I'm manually tested some of these test cases that changed, and the ones I've looked at still appear to work, so it might not be all bad.

Note: Some of the output.js files behaved strange for me in that it'll keep receiving different values depending on what it's expecting, so it's impossible to update the test to make it pass even by deleting the snapshot. I.e. this test below, if the expected value is set to the received, then it should pass, but suddenly a new value is received:


// __dom_fixtures__/fragments/output.js

-- const lastDynamic = [_tmpl$3(), _$memo(() => inserted)];
++ const lastDynamic = [_tmpl$3(), _$memo(inserted)];
ryansolid commented 4 months ago

The biggest risk is things that might work, but won't update. There is also the chance of weird recursion due to passing functions up the chain. Like the reason we memo in fragments is to prevent unnecessary execution. Having a memo return a function in that case will work but some unrelated change can cause other stuff.

ryansolid commented 4 months ago

I think this was just a bug. https://github.com/ryansolid/dom-expressions/commit/eeca96004070473aed0e3454423ce164f0a5e7d2#diff-6971a60935cf3ec66903ea31c0bb58c4667e48f29a9e71aacdc2a4ba51745295R133

We have 2 different modes for compilation here. One that leverages what I call onioning which is slightly more performant as it can avoid remaking memos and one that inlines it. We were using inline for components and we should have been for fragments as well.

birkskyum commented 4 months ago

Excellent! That's a better output:

// Before
  return _$memo2((() => {
    const _c$ = _$memo2(() => !!obj2.prop);
    return () => _c$() ? (() => {
      const _c$2 = _$memo2(() => !!obj2.prop);
      return () => _c$2() ? _tmpl$() : [];
    })() : [];
  })());

// After
  return _$memo(() =>
    _$memo(() => !!obj1.prop)() ?
    (_$memo(() => !!obj2.prop)() ?
      _$getNextElement(_tmpl$2) : []) : []
  );

Think maybe the pnpm lock file got out of sync with the new releases