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

Gathering @developit's suspense hydration work #2259

Closed
wants to merge 22 commits into from

Conversation

prateekbh
Copy link
Member

@prateekbh prateekbh commented Jan 17, 2020

@coveralls
Copy link

coveralls commented Jan 17, 2020

Coverage Status

Coverage increased (+0.001%) to 99.809% when pulling 7f844b1 on prateekbh/suspense-hydration into 0fb3b13 on master.

src/diff/index.js Outdated Show resolved Hide resolved
Copy link
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try and get some tests for this, have we tested this in a real-world scenario? I can't really see any flaws in this at the moment, so really amazing job here 🎉

@prateekbh prateekbh changed the title Prateekbh/suspense hydration Gathering @developit's suspense hydration Jan 18, 2020
@prateekbh prateekbh changed the title Gathering @developit's suspense hydration Gathering @developit's suspense hydration work Jan 18, 2020
Copy link
Member

@cristianbote cristianbote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is amazing! 🎉

@prateekbh
Copy link
Member Author

I added the test case for out scenerio.
But in the process I discovered another bug

Use case 1

<div id="test">
  <Suspense>
    <Component />
  </Suspense>
  <p>Hello foo</p>
</div>

This is our current use case where Component is lazy loaded but its markup is not deleted thus

<div id="test">
  <p>Component</p>
  <p>Hello foo</p>
</div>

is the markup while Component is being loaded.

Use Case 2

<div id="test">
  <Suspense fallback={<p>fallback</p>}>
    <Component />
  </Suspense>
  <p>Hello foo</p>
</div>

Now since the markup is not being deleted the html would be

<div id="test">
  <p>fallback</p>
  <p>Component</p>
  <p>Hello foo</p>
</div>

This is where the problem is, both fallback and the actual markup are shown on the screen.

Since most Suspense are in practice written with fallback this'd be a problem.

@prateekbh
Copy link
Member Author

Either our solution should satisfy both cases or the behavior we're targeting with this PR should be achievable with some hook but should not be the default one.

@prateekbh prateekbh changed the title Gathering @developit's suspense hydration work [DO NOT MERGE] Gathering @developit's suspense hydration work Jan 20, 2020
@prateekbh
Copy link
Member Author

prateekbh commented Jan 21, 2020

IMHO preserving of DOM when a promise is thrown should NOT be the default behavior as this breaks the Suspense rendering and makes it unintuitive for a large number of use cases. But preact should have options/API hooks extensible enough to achieve the same.

@prateekbh prateekbh changed the title [DO NOT MERGE] Gathering @developit's suspense hydration work Gathering @developit's suspense hydration work Jan 22, 2020
@prateekbh
Copy link
Member Author

Ok this is now fixed I feel.
Fix: During hydration, suspense components will not render a fallback component. It will show the pre-rendered component markup and then hydrate it once the Suspense resolves.

If nothing is pre-rendered in place of Suspense component then it will just inject the component when the Suspense resolves.

compat/src/suspense.js Outdated Show resolved Hide resolved
src/diff/index.js Outdated Show resolved Hide resolved
@prateekbh prateekbh requested a review from developit January 22, 2020 17:22
Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff! 🙌 💯

@github-actions
Copy link

github-actions bot commented Jan 24, 2020

Size Change: +207 B (0%)

Total Size: 38.5 kB

Filename Size Change
compat/dist/compat.js 2.97 kB +9 B (0%)
compat/dist/compat.module.js 2.99 kB +7 B (0%)
compat/dist/compat.umd.js 3.02 kB +10 B (0%)
dist/preact.js 3.75 kB +46 B (1%)
dist/preact.min.js 3.75 kB +46 B (1%)
dist/preact.module.js 3.77 kB +46 B (1%)
dist/preact.umd.js 3.81 kB +43 B (1%)
ℹ️ View Unchanged
Filename Size Change
debug/dist/debug.js 3.08 kB 0 B
debug/dist/debug.module.js 3.06 kB 0 B
debug/dist/debug.umd.js 3.14 kB 0 B
devtools/dist/devtools.js 175 B 0 B
devtools/dist/devtools.module.js 185 B 0 B
devtools/dist/devtools.umd.js 252 B 0 B
hooks/dist/hooks.js 1.06 kB 0 B
hooks/dist/hooks.module.js 1.08 kB 0 B
hooks/dist/hooks.umd.js 1.12 kB 0 B
test-utils/dist/testUtils.js 390 B 0 B
test-utils/dist/testUtils.module.js 392 B 0 B
test-utils/dist/testUtils.umd.js 469 B 0 B

compressed-size-action

@squidfunk
Copy link

This looks promising and I can add another use case: I’m using Suspense without lazy to throw a fetch promise in a data loading component (in componentDidMount). Is it possible to somehow detect that the component was pre rendered and avoid the fetch call?

@developit
Copy link
Member

@squidfunk Avoiding the fetch call seems difficult - how would you get the data loaded otherwise?

If you're just looking for a way to indefinitely hold off rendering part of your app, this should likely give you that option. All you have to do is throw a pending Promise:

const data = fetch('/data.json')
    .then(r => r.json())
    .then(value => data.value = value);
function MyComponent() {
  // don't render this subtree until we have data
  if (!data.value) throw data;

  return <pre>{JSON.stringify(data.value,0,2)}</pre>
}

// somewhere up the tree:

class MyApp extends Component {
  componentDidCatch(err) {
    if (err instanceof Promise) {
      err.then(() => this.setState({}));  // or forceUpdate
    }
  }
}

@squidfunk
Copy link

@developit thanks for your response! Maybe I'm missing something but I may have found a way making things work.

What I want to do is: separate article content in a Preact application (i.e. blog) from application logic. I'm using htmdx to build Preact's VNode intermediate representation and serialize it to JSON. The blog then requests the JSON and passes the nodes dynamically to h. Everything works as expected, except with SSR, as the actual DOM nodes are already there. What I want is to skip the fetch and just mount the event handlers registered with the components. However, I realize that Preact needs to know what DOM nodes are rendered so the only way to do this is to inline the JSON intermediate representation with the pre-rendered Markup, check if it's available and only fallback to the fetch when it is not. Or is there an easier way?

Sorry for hi-jacking this thread but I though it was somehow related. Happy to move the discussion somewhere else if necessary.

@developit
Copy link
Member

developit commented Jan 27, 2020

@squidfunk technically you could create a Virtual DOM tree from the existing DOM tree. There's actually a module that does that exact thing over in preact-markup:

import toVdom from 'preact-markup/src/to-vdom.js';

let cached;
function AbsorbDom() {
    if (cached) return cached;
    return cached = toVdom(
        document.querySelector('article'),  // your article's root element
        vnode => {},                        // optional modify the created Virtual DOM elements
        h,                                  // preact's h()
        { trim: false }                     // preserve whitespace
    );
}

You're right about this being slightly off-topic - maybe better to migrate to Slack?
We're over at https://preact-slack.now.sh

@andrewiggins
Copy link
Member

FYI, one of my recent completed PRs conflicted with this one so I fixed for ya

@ForsakenHarmony
Copy link
Member

rebasing when it's ready makes more sense to me than merging master into it all the time, but idk 🤷‍♀️

@prateekbh
Copy link
Member Author

we discovered that there were false passing tests and that this approach doesn't work. @developit is working on an alternate approach in #2290 and #2291.
Closing this one

@prateekbh prateekbh closed this Feb 3, 2020
@ForsakenHarmony ForsakenHarmony deleted the prateekbh/suspense-hydration branch May 21, 2021 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants