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

[wip] experimenting with the new suspense hooks #7010

Closed
wants to merge 1 commit into from

Conversation

@ryanflorence
Copy link
Member

ryanflorence commented Oct 25, 2019

React Router + Suspense

In order for React Router to support suspense we need a few pieces.

Convert classes to functions

We need to use the useTransition hook to allow suspense boundaries to actually suspend. While suspense will still work with our class components, they never suspend, they always go to the loading state.

This will be a breaking change for <BrowserRouter/> and friends, so we either do it in 6.0 or we ship two versions of everything (Router, BrowserRouter, HashRouter, etc.).

Update history to replace calls to push while suspended

We already know this. When a user clicks a link, then clicks another link before the resources are done loading, they'll end up with ghost entries in the history stack--meaning they can click "back" and see an unexpected page.

But we can do more!

Rather than just support it, we can actually help manage resources on route changes, and probably with a tiny API footprint.

NOTE: I'm just going for an API design idea here, the implementation has many problems that we can solve with stuff we've been working on but haven't released yet, so don't worry about the implementation too much.

Provide a simple entry point to preload data for the route

The React team's experience with suspense has shown that using render to kick off fetches in the component that needs the data leads to too many waterfalls. While they had hoped people would preload in some top-level component, they didn't. It would be unfortunate if suspense actually caused longer load times because the API encouraged waterfall requests.

The vast majority of data fetches in an app are initiated on route changes, so we're in a great position to help developers out. In fact, I was able to implement a very quick proof-of-concept in a few minutes, check it out:

<Route preload={(params, location) => resource} />

This prop will be called on render whenever a route matches and mounts. It passes in the params and location and expects a resource of any shape to be returned. It's up to the app to determine the api for the resource, we just provide a way to kick of a preload of it.

It might look something like:

const resource = createResource(key)

// Then if you try to read from it and it's not yet resolved,
// it will throw the promise to trigger suspense
resource.read()

So we might have a route like this:

<Route
  path="/invoice/:invoiceId"
  preload={(params) => createInvoiceResource(params.invoiceId)}
>
  <Invoice />
</Route>

useResource()

This hook is used inside a component to get access to the resource returned in the <Route preload/> prop. It will find the nearest resource and return it. Route components can use this hook to read data from the resource. Continuing with the invoice example:

import { useResource } from 'react-router-dom'

function Invoice() {
  const invoice = useResource().read()
  // returns the invoice if resolved, triggers suspense if not
  // ...
}

useResourcesPending()

This indicates that resources are being loaded. While suspense has a timeout before falling back or transitioning to a partial page, sometimes you want to add loading indicators to the first page. The useTransition hook returns an isPending value. This simply gives route components access to it.

const isPending = useResourcesPending()

That's it! Check out the demo in /fixtures/suspense

@sibelius

This comment has been minimized.

Copy link
Contributor

sibelius commented Oct 25, 2019

you don't want to create a new resource on each preload

you want to reuse a resources and its cache

you want something like relay preloadQuery (https://relay.dev/docs/en/experimental/api-reference#usepreloadedquery), where you going to preload data and consume it using resource.read on render

useResourcesPending could have another name, like isPendingTransition

@timdorr

This comment has been minimized.

Copy link
Collaborator

timdorr commented Oct 25, 2019

useResourcesPending could have another name, like isPendingTransition

It's a hook, so it should be prefixed with use. useAreResourcesPending?

@sibelius

This comment has been minimized.

Copy link
Contributor

sibelius commented Oct 25, 2019

usePendingTransition

@mjackson

This comment has been minimized.

Copy link
Member

mjackson commented Oct 25, 2019

Very cool, thanks for sharing :)

I believe <Route preload> suffers from the waterfall problem, no? At least in the current router design it does. With useRoutes we can do all the preloading together. But with <Switch> we have to go one level deep at a time.

you don't want to create a new resource on each preload. you want to reuse a resources and its cache

He's already doing that, @sibelius. See here.

@sibelius

This comment has been minimized.

Copy link
Contributor

sibelius commented Oct 25, 2019

you can solve request waterfall of nested routes using Entrypoint approach (facebook/relay@861990a)

similar to matchRoutes (https://github.com/ReactTraining/react-router/blob/master/packages/react-router-config/README.md#matchroutesroutes-pathname)

where you could preload all data dependencies for each nested routes before rendering them

@MeiKatz

This comment has been minimized.

Copy link
Contributor

MeiKatz commented Oct 25, 2019

I might be wrong, but currently we are call preload and indepentent from its return value, we render the component inside the <Route /> and pass the return value via useResource. I don't think that this is an improvement compared to the "current" usage of the suspense api. Either we should do something similiar to the suspense api (throw promise => return value / throw exception) or should only accept promises as return value. Anyway, we should wait for the resource to be loaded before we render what's inside the <Route />.

@ryanflorence

This comment has been minimized.

Copy link
Member Author

ryanflorence commented Oct 25, 2019

To be clear, just going for an API design here, the implementation has many problems and some new stuff we've been working on will enable us to fix them if we go this direction.

@ryanflorence

This comment has been minimized.

Copy link
Member Author

ryanflorence commented Oct 25, 2019

I don't think that this is an improvement compared to the "current" usage of the suspense api

@MeiKatz We're not trying to wrap the suspense API in something "better", just providing a way for the parent to fire off fetches on route changes. The demos in the docs do this on button click, we do it when matching routes, but we're not replacing any suspense APIs here.

Anyway, we should wait for the resource to be loaded before we render what's inside the <Route />.

It's not our job to "wait" for the data, the app gets to decide, that's what Suspense is all about!

@mjackson

This comment has been minimized.

Copy link
Member

mjackson commented Oct 25, 2019

Based on @josephsavona's talk at React Conf this morning, we're thinking the API for integrating with Relay's new experimental API could be as simple as:

// First, in your route.preload you need to preloadQuery
<Route
  path="users/:id"
  preload={params => preloadQuery(usersQuery, { id: params.id })}
>
  <UserProfile />
</Route>

// Then, in your route component, use that data
function UserProfile() {
  let user = usePreloadedQuery();
  // go for it!
}

If Apollo follows suit, we'll be able to integrate with them just as easily.

@timdorr

This comment has been minimized.

Copy link
Collaborator

timdorr commented Oct 25, 2019

@mjackson How does that prevent waterfall?

Say you've got a user profile with tabs for their posts and friends. You go to a URL like /users/123/posts to get to the User > Posts tab.

If you load up that URL, you want to start a single (or at least simultaneous) data fetch to get the user and their posts. If you click to the friends tab, you want to make a fetch to get just the comments, as you already have the user data. Luckily, that's already handled.

There needs to be a way to inform tree ancestors that you need to "add on" to the preload. We do this on React Redux with a Subscription class that allows you to have a parent subscription (all the way up to the root subscription on <Provider>). We do this for other reasons (ordering of updates so we don't have stale, "zombie" children in the tree), but the idea is the same.

Would that pattern be a potential solution to that problem?

@sibelius

This comment has been minimized.

Copy link
Contributor

sibelius commented Oct 25, 2019

what about nested routes?

@sibelius

This comment has been minimized.

Copy link
Contributor

sibelius commented Oct 29, 2019

nice example relayjs/relay-examples#104 of how router works together with suspense

@josephsavona

This comment has been minimized.

Copy link

josephsavona commented Oct 29, 2019

I really like the idea of adding a preload() or prepare()-style method with each route. However, I ran into a few problems with trying to implement Suspense integration (including useTransition) when using react-router that are relevant to the discussion here, so I hope you don't mind if I share a bit about those issues and how I solved them.

First, I have always found the JSX-based route config very difficult to reason about. The object-based route config style supported by react-router-config matches my mental model so much better, so I just started there. For each entry I defined a component property that was either a React component or a JSResource instance (that has load(): Promise<Component> and read(): Component methods - where read suspends while pending / throws on error). Then I tried using BrowserRouter and defining an App component that called useLocation(). It was along the lines of:

// NOTE: this approach is problematic - see issues below!!
function App() {
  const location = useLocation();
  const[currentLocation,setLocation]=useState(location);
  if (location.pathname !== currentLocation.pathname) {
    setLocation(location);
  }
  const pathname = currentLocation.pathname;
  // the issue here is that memoization only kicks in *after* a component commits
  // so the component/data preparation logic will keep happening until the tree commits
  // this work should happen *outside* React, in the route change handler.
  const {component, prepared} = useMemo(() => {
    // route entries are:
    // { component: JSResource
    //   prepare: params => mixed }
    const matchedRoute = matchRoutes(routes,  pathname);
    // returns {component, prepared} where `prepared` is results of prepare(routeParams)
    return prepare(matchedRoute); 
  }, [pathname]);
  return (
    <ErrorBoundary>
      <Suspense>
        <RouteComponent component={component} prepared={prepared} />
      </Suspense>
   </ErrorBoundary?
  );
}

function RouteComponent({component,prepared}) {
  const Component =  component.read(); // may suspend/throw; otherwise returns component
  return <Component prepared={prepared} />;
}

There are a few problems though. First, BrowserRouter itself calls setState, but this can trigger a re-render that suspends (e.g. when RouteComponent read()s the route's component or when that component tries to read its still-loading data). More importantly, though, this has many of the complications of fetch-on-render. In concurrent mode, App might render multiple times before it commits, or render and never commit. Note that useMemo doesn't work until a component has committed - so in practice, the preloading work in the useMemo hook will happen multiple times - not ideal! Also, the entire routing context object changes frequently, even when the route itself hasn't changed. Again, it's hard to avoid this in concurrent mode bc the only place to store temporary state is the component, and that may not be recycled when components suspend.

What I ended up doing is the following:

  • Define my own RoutingContext
  • Define a createRouter(routes) function that takes a route config (of the shape expected by react-router-config), where each entry has component and prepare() as above.
  • Implement the preloading logic within createRouter() itself. So rather than the UI listening for changes to the location, it instead listens for changes to the current routing entry, which includes the already prepared data for that entry.
  • Then, RouteRenderer just consumes the active route entry, rendering it with the same <RouteComponent> API shown above. It maintains the current route entry with useState(), but wraps state updates in useTransition to continue showing the previous route for a brief period while the next route is prepared.

You can see all of this in the ./src/routing/ directory in relayjs/relay-examples#104.

I certainly don't have full context on all the use-cases that react-router is trying to support and issues such as incremental adoption, but from a new-user and concurrent/suspense perspective, having the above approach baked in would be really nice to use. I just define each route entry w its component and prepare function, and then the component loading and prepare() happens once when the route changes (outside of React), and the component gets rendered with the prepared data passed as a prop. No need for context or useResource hooks.

@mjackson

This comment has been minimized.

Copy link
Member

mjackson commented Oct 29, 2019

rather than the UI listening for changes to the location, it instead listens for changes to the current routing entry, which includes the already prepared data for that entry

I think what you're saying here is that the difference between your example and this one is that you're doing your preloading when the route first matches instead of when it renders, is that right @josephsavona? Or are you suggesting there is a problem with storing the current location in state?

@josephsavona

This comment has been minimized.

Copy link

josephsavona commented Oct 29, 2019

@mjackson Basically yes: the key distinction is that for predictable behavior in concurrent mode - and for efficiency in starting the fetch earlier - preloading would ideally happen in the event handler for the location change, not in render. There's nothing inherently at issue with storing the location in context. But since the preload has to happen outside of render, the preloaded results also have to be able on context. Note how the implementation in the PR I linked only preloads code/data in the router itself, and makes that result available to context. If only the location is available on context, you have no choice but to preload the location in render.

@mjackson

This comment has been minimized.

Copy link
Member

mjackson commented Oct 29, 2019

Thanks for confirming, @josephsavona. We are on the same page. When we actually ship this feature we will not be preloading inside render 👍 I think this PR might be a bit misleading since we're essentially trying to hack the current router API to do what we want. But the demo you made has the right idea; match the routes, start preloading, then render.

@muqg

This comment has been minimized.

Copy link

muqg commented Nov 23, 2019

Since that we are going to have preloading, would it be possible to also have a feature like preload on hover for <Link /> and <NavLink /> components. I think it will be a nice opt-in addition for some cases.

I know that it can be implemented in userland, but it would be better if it worked by just flicking a prop on a Link component. Furthermore, the router will already know what to preload for the target route of a Link component.

@sibelius

This comment has been minimized.

Copy link
Contributor

sibelius commented Nov 23, 2019

check how to implement preload on on this pull request

relayjs/relay-examples#112

@stale

This comment has been minimized.

Copy link

stale bot commented Jan 22, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
You can add the fresh label to prevent me from taking any action.

@stale stale bot added the stale label Jan 22, 2020
@stale stale bot closed this Jan 29, 2020
@lukaspili

This comment has been minimized.

Copy link

lukaspili commented Jan 29, 2020

@ryanflorence Does it mean the suspense support is abandoned?

@oallouch

This comment has been minimized.

Copy link

oallouch commented Jan 29, 2020

I think they got enough info to do it. Don't worry :)

@MeiKatz

This comment has been minimized.

Copy link
Contributor

MeiKatz commented Jan 29, 2020

@lukaspili @oallouch I think they got caught by their own pull request policy and the corresponding bot ;)

@mjackson

This comment has been minimized.

Copy link
Member

mjackson commented Feb 4, 2020

This PR was an experiment, never supposed to actually be merged. It's ok the bot closed it for us. 😅

We are still working on multiple aspects of suspense integration in v6. You can follow along on the dev branch if you'd like to be notified when stuff lands.

@otakustay

This comment has been minimized.

Copy link

otakustay commented Feb 6, 2020

I like the preload feature, but I don't want preload implementation to be bound within react-router's API like useResource, I'd like to see a more flexible approach where we can handle preloading separately and communicates with react-router via Promise or another more widely used data structure.

In this case we can introduce any data fetching mechanism, or cache preloaded data in any level and serve it to presentation component via context or redux store

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.