Skip to content

feat(core): allow to preload additional resources before rendering a page#6637

Closed
koistya wants to merge 2 commits intofacebook:mainfrom
koistya:preload-arguments
Closed

feat(core): allow to preload additional resources before rendering a page#6637
koistya wants to merge 2 commits intofacebook:mainfrom
koistya:preload-arguments

Conversation

@koistya
Copy link
Contributor

@koistya koistya commented Feb 8, 2022

Motivation

Need a way to load additional resources, e.g. fetching (critical) data from a REST/GraphQL API, before rendering a page on the client side. As opposed to showing a loading indicator on the screen during data fetching. You can always combine it with progressive rendering of UI elements using React Suspend.

Approach 1:

If src/preload file exists (by convention), Docusaurus would call it during client-side preloading:

routes.addRoute({
  path: "/prices/:symbol",
  component: "@site/src/Prices.tsx",
});
// src/preload.ts
export default async ({ route, match }) => {
  switch (route.path) {
    case "/prices/:symbol":
      const res = await fetch(`https://rates.sx/${match.params.symbol}`);
      const price = await res.text();
      return { price };
    default:
      return undefined;
  }
}
// Route/page component (example)
function Prices({ match, data }) {
  return (<p>{match.params.symbol} Price: {data.price}</p>);
}

Approach 2:

It can be implemented by allowing to pass an optional preload property into the .addRoute(...):

// Dynamic page route added via a plugin
routes.addRoute({
  path: "/prices/:symbol", // e.g. /price/BTC
  component: "@site/src/Prices.js",
  async preload({ params }) {
    const res = await fetch(`https://rates.sx/${params.symbol}`);
    const price = await res.text();
    return { symbol: params.symbol, price };
});
// src/Prices.tsx
export default function Prices({ match, data }) {
  return (<p>{data.symbol} Price: {data.price}</p>);
}

Have you read the Contributing Guidelines on pull requests?

Yes

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Feb 8, 2022
@koistya koistya changed the title Access the matched route info from under preload() feat(core): access the matched route info from under preload() Feb 8, 2022
@netlify
Copy link

netlify bot commented Feb 8, 2022

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 45ca38c
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/624fe7219f62250009dd998f
😎 Deploy Preview https://deploy-preview-6637--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Feb 8, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 91
🟢 Accessibility 100
🟢 Best practices 92
🟢 SEO 100
🟢 PWA 90

Lighthouse ran on https://deploy-preview-6637--docusaurus-2.netlify.app/

@Josh-Cena Josh-Cena added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Feb 9, 2022
@slorber
Copy link
Collaborator

slorber commented Feb 9, 2022

Have you actually tested the code of this PR? Because to me it doesn't work 😅


We use preload on the static pages with React-Loadable components, which do not take these extra args: https://github.com/jamiebuilds/react-loadable#preloading

In practice, if you provide your own "preload" callback to a page component, I suspect only one of these 2 will be called:

  • the react-loadable fn
  • your function

I tried this on our own homepage:

Home.preload = async function () {
  console.log('Home.preload');
};

Now if I run window.docusaurus.preload("/")

I only see react-loadable preload being called, not the homepage callback:

image

So it seems React-Loadable doesn't "hoist" your preload function, and just overrides it somehow.


Currently, there is no way to prefetch data before a dynamic (page) component is rendered (during navigation).

Please provide a better explanation of why you need this. What's the use-case and the expected business outcome?


let price = null;

// Dynamic page component (example)
function Price({ match }) {
  return (<p>{match.params.token} Price: {price}</p>);
}

// Load data from the API before rendering the page
Price.preload = async function({ match }) {
  const res = await fetch(`https://rate.sx/${match.params.token}`);
  price = await res.text();
};

export default Price;

I'd strongly advocate against using such a code with side effects variable stored as a global.

How would errors be handled exactly? Should we still navigate to the targeted page, or block the navigation?

How first render would work? Where is "preload" called in this case?

Imagine this scenario:

  • Navigate to /price/BTC, load BTC price and display it
  • Navigate to /price/ETC, load ETH price failed and it still displays BTC price because it's stored globally

I see how this feature could be useful for Docusaurus dynamic pages, but I think it's too early to create a PR for this.

We should try to adopt the render-as-you-fetch pattern of upcoming React versions, and I'd rather delay the implementation until then

https://reactjs.org/docs/concurrent-mode-suspense.html#approach-3-render-as-you-fetch-using-suspense

@koistya
Copy link
Contributor Author

koistya commented Feb 9, 2022

@slorber good point :) I'm trying to find a way to use Docusaurus for building single-page apps — having some dynamic routes, end being able to preload data (from API) for these dynamic routes, similarly to how Docusaurus preloads application chunks and shows a progress bar before the page renders on the screen.

I see that it calls route.compoentn.preload() here:

if (component && component.preload) {
// @ts-expect-error: checked above.
return component.preload();
}

That might be a good place to pass routing information (params) into the "loader". Next step is to figure out how to pass custom loaders into ComponentCreateor...

It's a bit ticker to implement with React Router (as opposed to using router optimized for pre-rendering and SSR).

This approach might be a good way to go:

// Dynamic page route added via a plugin
routes.addRoute({
  path: "/prices/:symbol", // e.g. /price/BTC
  component: "@site/src/Prices.js",
  async preload({ params }) {
    const res = await fetch(`https://rates.sx/${params.symbol}`);
    const price = await res.text();
    return { symbol: params.symbol, price };
});

or, even better, support Relay queries out of the box. Or, allow Docusaurus users to implement it this way:

addRoute({
  path: "/prices/:ticker",
  component: "@site/src/Prices.tsx",
  query: graphql`
    query RoutesPricesQuery($ticker: String!) {
      prices(ticker) { ...Prices_data }
    }
  `
}

https://github.com/kriasoft/relay-starter-kit/blob/71462f8c707200b81800c13063147e01799dc6e6/web/routes/user/index.ts#L14-L35

@slorber Q: Is there currently a way to suspend page rendering (by using Suspend or whatnot) before the data required by the route/page was loaded from the server (for dynamic pages/routes, instead of showing Loading... screen)?

@koistya koistya changed the title feat(core): access the matched route info from under preload() feat(core): pass route params to component.preload() Feb 9, 2022
@slorber
Copy link
Collaborator

slorber commented Feb 9, 2022

addRoute({
  path: "/price/:ticker",
  component: "@site/src/Price.tsx",
  // this function would be passed into the generated code → react-loadable
  preload: `({ params }) => fetch(\`https://rate.sx/api/${params.ticker}\`).then(res => res.text())`
});

That does not really look like a good API 😅


@slorber Q: Is there currently a way to suspend page rendering (by using Suspend or whatnot) before the data required by the route/page was loaded from the server (for dynamic pages/routes, instead of showing Loading... screen)?

No good way for now no, most users are using Docusaurus for static pages.


I still don't understand how you want to handle those cases exactly:

  • first load: how can we avoid displaying a spinner if the page is not statically rendered? In all case you'd have to keep handling the unavailability of data in your JSX
  • navigation: what happens if your promise / fetch() fails, timeouts or takes like 1min for some weird reason?

@koistya koistya force-pushed the preload-arguments branch 2 times, most recently from ed6b353 to 5f178a0 Compare February 9, 2022 18:31
@koistya
Copy link
Contributor Author

koistya commented Feb 9, 2022

@slorber regarding these use cases:

first load: how can we avoid displaying a spinner if the page is not statically rendered? In all case you'd have to keep handling the unavailability of data in your JSX

It would display the "Loading" indicator in the toolbar, but the new (page) component is not rendered on the screen until all the required stuff is loaded. The exact same way it works when preloading application chunks — you don't show Loading in the middle of the screen while chunks are being loaded. We need the exact same behavior for preloading minimum necessary data from the API for same (dynamic) routes/pages.

navigation: what happens if your promise / fetch() fails, timeouts or takes like 1min for some weird reason?

It would render Docusaurus error page, unless the route has it's own error boundary in place.

That does not really look like a good API 😅

Yeah, the API can be better, but most likely it would require replacing React Router with an alternative solution optimized for pre-rendering / SSR 😉 I'm curios to see your API proposal for this feature.

@koistya koistya changed the title feat(core): pass route params to component.preload() feat(core): allow to preload additional resources before rendering a page (WIP) Feb 9, 2022
@Josh-Cena Josh-Cena marked this pull request as draft February 10, 2022 01:33
@Josh-Cena Josh-Cena changed the title feat(core): allow to preload additional resources before rendering a page (WIP) feat(core): allow to preload additional resources before rendering a page Feb 10, 2022
parts.push(`${key}: ${JSON.stringify(propValue)}`);
const value =
typeof propValue === 'function'
? propValue.toString()
Copy link
Collaborator

@Josh-Cena Josh-Cena Feb 10, 2022

Choose a reason for hiding this comment

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

I don't like this API. It breaks the isolation of client-server and means plugin authors can write arbitrary JS code that's executed on the client side, which can be easily error-prone. This has never been the case before, and I don't feel like it's good.

@slorber
Copy link
Collaborator

slorber commented Feb 10, 2022

@koistya I'm not sure to understand the real motivation behind this feature.

What are the desired business outcomes? Why is it important to achieve it? Why it's not possible without this feature? Please share a very concrete use-case.

This feature looks like a potential nice to have for some UX edge cases (ie for users using dynamic pages, not a lot), but is not essential IMHO

We are trying to focus on 2.0, so we can't dedicate too much time on this PR in the short term.
And we won't rush on merging such a new feature without clearly understanding how much this is needed and why.


I also suggest that you implement a POC end-2-end: please try to add this feature on a page in the _dogfooding folder (or eventually even the homepage) so that we can see the final UX that your feature would unlock. You can use a fake async callback with a random delay.

It's better if we can see it in action before deciding if it's good or not on paper

@koistya
Copy link
Contributor Author

koistya commented Feb 10, 2022

@slorber I see that some efforts have been made to make Docusaurus work for building SPAs (with its ability to add wildcard routes). This feature is critical to make that kind of routes work smoothly (without flashing empty or loading screen as you navigate from page to page), assuming that these routes need to fetch data from API before they can render some meaningful UI — basically, the exact same way it works with application chunk preloading.

@Josh-Cena
Copy link
Collaborator

Josh-Cena commented Feb 10, 2022

I see that some efforts have been made to make Docusaurus work for building SPAs (with its ability to add wildcard routes)

That's only an auxiliary benefit of exposing the whole react-router-config API. The plugin can insert any kind of route config, some of which may not have first-class support by us.

Still, with that said, I see the benefit of being able to prefetch/preload dynamic routes.

@slorber
Copy link
Collaborator

slorber commented Feb 10, 2022

@koistya again, please provide a real use-case: show us a real page, with dynamic data, where spinners are not good. What do you want to display on that page exactly, and why exactly do you think the current Docusaurus experience is not great.

Help us feel the problem you have, provide a real demo where we can see the problem that we are trying to solve in practice.

assuming that these routes need to fetch data from API before they can render some meaningful UI

In any case, you have to render spinners for the very first load because the page must be statically rendered. The only other option is to render a blank page with a progress bar until React hydrates 🤷‍♂️ this is definitively not good, and spinners would rather be inlined in the HTML file.

@koistya
Copy link
Contributor Author

koistya commented Feb 10, 2022

please provide a real use-case: show us a real page, with dynamic data, where spinners are not good.

Literally any single-page app. Note, that we're talking specifically about wildcard routes here and/or client-side (only) page rendering.

Take a look at this Docusaurus GitHub repository as an example, as you click on the navigation and go from [Source], to [Issues], to [Pull Requests], it shows a loading indicator in the toolbar but does not render the next page until it has all the required data in place.

Currently, you have this implemented for pre-loading missing application chunks before rendering the next page/route. If it would allow loading some other resources before rendering the page, that would make the wildcard routes feature more useful (whipping out the whole screen as the user goes from page to page definitely won't be a good user experience).

@slorber
Copy link
Collaborator

slorber commented Feb 11, 2022

Literally any single-page app.

Again, I don't ask for a "kind of app", but for one very concrete and specific example.

Take a look at this Docusaurus GitHub repository as an example, as you click on the navigation and go from [Source], to [Issues], to [Pull Requests], it shows a loading indicator in the toolbar but does not render the next page until it has all the required data in place.

Unlike Docusaurus, GitHub is a server-rendered app. It may provide turbolinks/progress on page transitions, but it is a very different case because it never has to deal with the first load experience. GitHub never has to render spinners because the first load always contains correct HTML markup.

Docusaurus will have to render spinners for dynamic pages because it can't render those pages on the server, even if they contain a wildcard and are dynamic routes.


If you want this feature to land, please do what I ask:

@Josh-Cena
Copy link
Collaborator

Note, the ability to not render spinners does not entirely come from preloading, but also from PendingNavigation. If you remove that intercepter, users with slow network will still see loading screens. And PendingNavigation is always working regardless of dynamic or fixed routes.

@slorber
Copy link
Collaborator

slorber commented Apr 8, 2022

this PR is not ready and React Router will soon support this feature out of the box (https://remix.run/blog/remixing-react-router)

We'll want to use the RR support directly instead of ending up with our own system alongside the one RR will provide

@slorber slorber closed this Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Signed Facebook CLA pr: polish This PR adds a very minor behavior improvement that users will enjoy.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants