Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Comments on the v6 preload API #7200

Closed
chrisvasz opened this issue Mar 18, 2020 · 21 comments
Closed

Comments on the v6 preload API #7200

chrisvasz opened this issue Mar 18, 2020 · 21 comments

Comments

@chrisvasz
Copy link

TL;DR

The current v6 preload proposal appears difficult to statically type. Here is a proposal for an API that would be easier to type correctly. I implemented a small proof-of-concept using Flow (we don't use TypeScript) and everything looks good.

Current API proposal

I know the v6 preload API is still very early, but I'd like to contribute some thoughts to the discussion. If I'm understanding correctly, the proposal for the current preload API looks something like this:

import { Route, useResource } from 'react-router'

<Route
  preload={routeParams => aResource}
  element={<Component />}
/>

function Component() {
  let resource = useResource()  // access the return value from the preload call
  ...
}

We use static typing extensively in our application and I'm struggling to envision a way to type the return value of useResource without resorting to an ugly type cast that is not statically verifiable.

Statically-typeable API proposal

Here is an example API that would be much easier to statically type:

import { Route } from 'react-router'

<Route
  preload={routeParams => aResource}
  element={resource => <Component resource={resource} />}
/>

function Component({ resource }) {
  ...
}

The element prop accepts either a React element or a function that accepts the resource (returned from preload) and returns a React element.

To avoid prop-drilling, we could also pass the resource through context:

import { Route } from 'react-router'

<Route
  preload={routeParams => aResource}
  element={resource => (
    <Resource.Provider value={resource}>
      <Component />
    </Resource.Provider>
  )}
/>

function Component() {
  let resource = useContext(...)
  // could also accept resource as a prop and render the provider in here
}

Full example

Here is a full example inspired by some of our application code, including our take on the "render-as-you-fetch" pattern. The application code is from a chat UI.

import { queryCache, useQuery } from 'react-query'

function fetchThreads() {
  // threads fetching code
}

function prefetchThreads() {
  queryCache.prefetchQuery(fetchThreads)
}

function makeThreadsResource() {
  prefetchThreads()
  return function useThreads() {
    // This hook is embedded and returned here so that it is
    // inaccessible outside a call to makeThreadsResource().
    // This enforces the "render-as-you-fetch" pattern.
    return useQuery(fetchThreads).data
  }
}
 
<Route
  preload={() => ({ useThreads: makeThreadsResource() })}
  element={resource => <Threads {...resource} />}
/>

function Threads({ useThreads }) {
  let threads = useThreads()
  ...
}

I would be happy to share my proof-of-concept Flow typings if they would help.

Other Considerations

  • The preload function would need to be written so that it is callable multiple times. At minimum, it would need to be called during the route's render so that its return value can be passed just-in-time to the element function (if it is a function and not a simple element). The application code inside the preload callback would take responsibility for caching network calls, etc.
  • Ideally, preload could be triggered by a customizable list of events, such as 'mousedown' on an associated <Link>. In that case, I could conceive of the preload function being called at least twice: once inside the mousedown handler and again when the route actually renders.
  • If we want to trigger a side-effect when the route starts to match, the preload callback is NOT the place for that effect. A better place would be an onEnter callback like RRv3.
@nimaa77
Copy link
Contributor

nimaa77 commented Apr 7, 2020

also, one more thing.
imagine we have a route config like this:

[
  {
    path: "/",
    element: Foo,
    preload: fooResource,
    routes: [
      {
        path: "/"
        element: Bar,
        preload: barResource
      },
      {
        path: "/baz"
        element: Baz,
        preload: bazResource
      },
      {
        path: "/qux"
        element: qux,
        preload: quxResource
      }
    ]
  },
  {
     path: "/other",
     element: Other,
     preload: otheResource,
  }
]

if we move from /other to / we need to fetch fooResource and barResource and with current API of react-router-config this is possible.

but if we move from / to /baz we only need to fetch bazResource and not fooResource, there is no API for this in V5 and I think having an API for this is really useful...

@stale
Copy link

stale bot commented Jun 6, 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 Jun 6, 2020
@Motoxpro
Copy link

Thank you! Just implementing a Relay solution and I agree with the above. I would be great to have access to the return value of preload.

@maggo
Copy link

maggo commented Aug 6, 2020

Stumbled upon this while I'm researching how to implement a deferred page transition and loading indicator, eg. waiting for the lazy component (or some other data), before actually executing the page switch. As I understand it's currently not possible at all in the v6 beta version without preload and useLocationPending, right?

@mohe2015
Copy link

mohe2015 commented Aug 6, 2020

@maggo If you use react experimental you can use Suspense and useTransition. This works with react router 6 but does not preload (e.g. load on mouseover or so). You can show a loading indicator before navigation though.

@stale
Copy link

stale bot commented Oct 7, 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 Oct 7, 2020
@phaux
Copy link

phaux commented Oct 7, 2020

I didn't have chance to use the new react-router with preload yet, but if I had, I would probably do it like this:

import { queryCache, useQuery } from "react-query"

function fetchTodos() {
  return fetch("/todos").then(res => res.json())
}

function App() { 
  return (
    <Routes>
      <Route
        path="todos"
        preload={() => queryCache.prefetchQuery("todos", fetchTodos)}
        element={<Todos />}
      />
    </Routes>
  )
}

function Todos() {
  const todos = useQuery("todos", fetchTodos).data

  return 
}

@stale stale bot removed the stale label Oct 7, 2020
@MeiKatz
Copy link
Contributor

MeiKatz commented Oct 7, 2020

@phaux Yeah, I like this. But it should also pass the route params as argument of the callback function.

@Hellzed
Copy link

Hellzed commented Oct 7, 2020

@phaux I've been trying to do something really similar with Relay experimental, but I'm getting an error (Relay: 'loadQuery' (or 'loadEntryPoint') should not be called inside a React render function.).
If I understand this Relay error, somehow, the preload function is called during render, which shouldn't happen.

See this examples: https://github.com/Hellzed/hello-relay-react-router-experimental
It includes a "hello" GraphQL server for convenience, as well as a simple React + Relay experimental + React Router v6 experimental app. The Relay environment provider is located in the index module, while the router is in the App module.

@damikun
Copy link

damikun commented Oct 7, 2020

@Hellzed

Not possible since cannot be called during render or by render itself... I think this is issue from react-router related to relay...
You can workaround this by wrapping custom router and call it in use effect out of router...

const query = graphql`
  query AppHelloQuery {
    hello
  }
`;
function Hello({ queryReference }) {
  const data = usePreloadedQuery(query, queryReference);
  return <p>{data.hello}</p>;
}
function RouteWrapper({loadQuery,reference, ...rest}){
  useEffect(() => {
    loadQuery({})
  }, [loadQuery])
  return  <Route {...rest}/>
}
function App() {
  const [queryReference, loadQuery] = useQueryLoader(query);
  return (
    <Router>
      <Routes>
       <RouteWrapper
          loadQuery={loadQuery}
          path="/"
          element={
            <React.Suspense fallback="Loading...">
            {
              queryReference != null && <Hello queryReference={queryReference} />
            }
            </React.Suspense>
          }
        />
       </Routes>
    </Router>
  );
}

@Hellzed
Copy link

Hellzed commented Oct 8, 2020

@damikun Now I see!
Since the purpose of the preload prop reads as follows:

The <Route preload> prop may be used to specify a function that will be called when a route is about to be rendered. This function usually kicks off a fetch or similar operation that primes a local data cache for retrieval while rendering later.

Wouldn't it make sense for React Router v6 to always wrap the function passed to preload in useEffect or useCallback, since we're expecting it to trigger a fetch and/or a cache state update?

(Haven't tried it, but do you think there would be any adverse effect in wrapping this whole block in React.useEffect(): https://github.com/ReactTraining/react-router/blob/dev-experimental/packages/react-router/index.tsx#L662-L668 )

@stale
Copy link

stale bot commented Dec 7, 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 Dec 7, 2020
@RichardLindhout
Copy link

@damikun Now I see!
Since the purpose of the preload prop reads as follows:

The <Route preload> prop may be used to specify a function that will be called when a route is about to be rendered. This function usually kicks off a fetch or similar operation that primes a local data cache for retrieval while rendering later.

Wouldn't it make sense for React Router v6 to always wrap the function passed to preload in useEffect or useCallback, since we're expecting it to trigger a fetch and/or a cache state update?

(Haven't tried it, but do you think there would be any adverse effect in wrapping this whole block in React.useEffect(): https://github.com/ReactTraining/react-router/blob/dev-experimental/packages/react-router/index.tsx#L662-L668 )

Maybe it would be better to always call it inside the render function so we could hooks in it like queryLoader from Relay, but now it gives an error that the amount of hooks are not equal

@stale stale bot removed the stale label Dec 8, 2020
@RichardLindhout
Copy link

Maybe something like a RouteContextProviderWithPreload instead of RouteContext.Provider
https://github.com/ReactTraining/react-router/blob/dev-experimental/packages/react-router/index.tsx#L673

function RouteContextProviderWithPreload({route, parentParams , params, index, location }) {
   const  preloaded = route?.preload(params, location, index)
   return <RouteContext.Provider
        children={route.element}
        value={{
          outlet,
          params: readOnly<Params>({ ...parentParams, ...params }),
          pathname: joinPaths([basename, pathname]),
          route,
          preloaded
        }}
      />
}

We then could use hooks like we want inside of the preload function

    <Route
      path="todos"
      preload={(params) => {
        const [queryReference, loadQuery, disposeQuery] = useQueryLoader(query);
        React.useEffect(() => {
          loadQuery(LoadProjectQuery, { id: params.id });
        }, [params]);
        return queryReference;
      }}
      element={<Todos />}
    />

I would also like if the returned result would be provided to the child route

@RichardLindhout
Copy link

It would also be nice if <LinkWithPreload existed where the preload function would be called onMouseIn or onMouseOver (prop setting?)

inside the <LinkWithPreload we would have problems with the approach described above. So I think @damikun approach would be better where the preload function is wrapped inside a useEffect

@asterikx
Copy link

asterikx commented Jan 28, 2021

Has somebody successfully integrated Relay with react-router using preloaded queries?

I have a ProjectRoute (that renders a Route and encapsulates the preloading logic) as part of my Routes. I can't get preloading to work. The query is null and there is no request being made. What's wrong with this code?

import React from 'react';

import { useQueryLoader } from 'react-relay/hooks';
import { Route } from 'react-router-dom';
import { Project, projectQuery } from '../pages/project';
import { ProjectQuery } from '../pages/project/__generated__/ProjectQuery.graphql';
import { RouteProps } from 'react-router';

type ProjectRouteProps = RouteProps;

function ProjectRoute(props: ProjectRouteProps) {
  const [query, loadQuery] = useQueryLoader<ProjectQuery>(projectQuery);
  console.log('render ProjectRoute - query is: ', query); // outputs: null

  return (
    <Route
      path="/projects/:id"
      element={query && <Project preloadedQuery={query} />}
      preload={({ id }) => loadQuery({ id })}
      {...props}
    />
  );
}

export { ProjectRoute };
function App() {
  // ...
  return (
    <Routes>
      {/*...*/}
        <ProjectRoute path="/projects/:id" />
      {/*...*/}
    </Routes>
  );
}

@tobias-tengler
Copy link

I'm also in favor of this proposal, this would integrate nicely with the query preloading of relay!

@radoslavkarlik
Copy link

Is there no way to currently preload in v6 beta?

@damikun
Copy link

damikun commented Apr 9, 2021

Is there no way to currently preload in v6 beta?

@radoslavk

You can preload but not with Relay since there is incompatibility between their implementations and relay functions.. You cannot call it from router preload... So if you don't need to call Relay or hook inside.. the preload will be fired...

@mjackson
Copy link
Member

mjackson commented Aug 9, 2021

Thanks everyone for weighing in on this issue. Our process for developing the preloading API in v6 will take into consideration how easy it is to ensure type safety between your data loaders and your component props. We definitely hear you there.

We have some plans to add data loading capabilities to React Router post v6, but probably not in the initial v6 stable release. It will probably land in 6.1 or 6.2. Right now we are just trying to limit the scope of v6 so we can get it shipped quickly.

@oallouch
Copy link

@mjackson At least we will get it. We can wait. Btw, thx for maintaining V3.

@remix-run remix-run locked and limited conversation to collaborators Sep 4, 2021
@chaance chaance closed this as completed Sep 4, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests