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

Apollo example: avoid double render in browser #4734

Merged
merged 1 commit into from
Jul 5, 2018

Conversation

NikitaVlaznev
Copy link
Contributor

@NikitaVlaznev NikitaVlaznev commented Jul 5, 2018

Apollo's getDataFromTree is supposed to be called during the server side rendering.
Being called in browser it fires an unnecessary fake render process and blocks components from rendering with loading=true.

Also there was a mistake in this code:

// `getDataFromTree` renders the component first, the client is passed off as a property.
// After that rendering is done using Next's normal rendering pipeline
this.apolloClient = props.apolloClient || initApollo(props.apolloState.data)

Apollo component is not rendered by getDataFromTree actually, it renders the App directly, thus props.apolloClient will always be undefined.

This example was discussed here: #387.

@timneutkens timneutkens merged commit 728871b into vercel:canary Jul 5, 2018
@Skaronator
Copy link
Contributor

Skaronator commented Jul 6, 2018

Thanks for your work however I've noticed a issue on the client when applying this PR.

Without the PR it'll wait for the result of the query before rendering the Page/changing the route.
https://streamable.com/vlaar

When applying the PR it'll change the route before loading the query. https://streamable.com/t6dit
Also the loading variable from Apollo is false which means that the query has been resolved (which can't be the case)

Maybe there is something wrong with my implementation.

The relevant code of the page:

class Series extends React.Component {
  static getInitialProps({ query: { id } }) {
   // This apply the query.id to this.props.id
    return { id };
  }

  render() {
    const { classes, id } = this.props;

    return (
      <Query
        query={QueryGetSeries}
        variables={{
          tvdbId: id,
        }}
      >
        {({ loading, error, data }) => {
          if (loading && !data) return 'Loading...';
          if (error) return `Error! ${error.message}`;
          if (!data || !data.getSeries) {
            return <Error statusCode={404} />; // Here it throws the 404 page that you can see in the video with this PR code
          }
          return (
            <Layout box>
              <PageTitle title={data.getSeries.name} />
            </Layout>
          );
        }}
      </Query>
    );
  }
}

The ID prop is coming from the link:

<Link href={`/series?id=${id}`} as={`/s/${id}`}>...</Link>

However I can confirm that it triggers render() only once (instead two times).

@NikitaVlaznev
Copy link
Contributor Author

"When applying the PR it'll change the route before loading the query", yes, and it is good, you go to a new page, it renders a Query children with loading state, makes a request and renders children with data. I consider this as a basic flow for apps with ApolloClient.

You can make some modifications to avoid displaying loading state, but only if you want to. This can be achieved by various prefetching techniques, one of them is a fake render in getInitialProps to fill the cache for the real render, but it will work only if you change pages, if you have several tabs on the same page, it won't work, you will get a loading state. One more technique is described in apollo docs.

About your implementation. It is strange, seems like it should work, may be somehow you are getting a not empty data in loading state, could you publish the whole app or log all Query's children props?

@Skaronator
Copy link
Contributor

The weird thing is that loading is false and error is undefined which basically means that the query was executed successfully but it didn't even executed the query at all at that point.

@NikitaVlaznev
Copy link
Contributor Author

NikitaVlaznev commented Jul 6, 2018

It seems that cache contains the requested data.

@Skaronator
Copy link
Contributor

Skaronator commented Jul 6, 2018

Oh looks like loading is actually true but my if () check doesn't work that way anymore since data is never undefined.

After changing it to this it'll actually displays a Loading... instead of 404.

- if (loading && !data) return 'Loading...';
+ if (loading) return 'Loading...';

But my approach with getInitialProps() doesn't work anymore. It'll change the route instant and shows the loading state. Anyway it's late and thanks for your input.

@NikitaVlaznev
Copy link
Contributor Author

If you want to prefetch data without showing loading state you can always use fake render on both server and client side.

@adamsoffer
Copy link
Contributor

@NikitaVlaznev One thing I noticed with this change is that you can no longer prefetch data. I use this example to prefetch data but it requires being able to run getDataFromTree on the client side. Any ideas on how to prefetch data if we only let getDataFromTree run on the server now?

@adamsoffer
Copy link
Contributor

adamsoffer commented Jan 2, 2019

I suppose you can fake render on the client with getDataFromTree in a prefetch function:

export const prefetch = async href => {
  // if  we're running server side do nothing
  if (typeof window === 'undefined') return

  const url = typeof href !== 'string' ? format(href) : href

  const { pathname } = window.location

  const parsedHref = resolve(pathname, url)

  const { query } = typeof href !== 'string' ? href : parse(url, true)

  const Component = await Router.prefetch(parsedHref)

  // if Component exists and has getInitialProps
  // fetch the component props (the component should save it in cache)
  if (Component && Component.getInitialProps) {
    const ctx = { pathname: href, query, isVirtualCall: true }
    const composedInitialProps = await Component.getInitialProps(ctx)
    await getDataFromTree(
      <Component ctx={ctx} {...composedInitialProps} />,
      {
        router: {
          asPath: ctx.asPath,
          pathname: ctx.pathname,
          query: ctx.query
        }
      }
    )
  }
}

@lock lock bot locked as resolved and limited conversation to collaborators Jan 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants