[DOCS] Docs should state that persistCache should be awaited

[adrienharnay]

Hey,

It is not clear in the documentation that persistCache is async and should be awaited.

await persistCache({
  storage: window.localStorage,
  cache,
});

Removing await breaks persistence, because the first queries will resolve before the cache is persisted, and won't resolve again when it is.

Could you please confirm that this is by design (or maybe something else is causing this and I'm wrong), and tell me if I should open a PR to clarify this in the docs?

Thanks!

[bostrom]

Also since persistCache is awaited, it needs to be wrapped in an async function, which propagates to App.js (or what have you) where the client is attached to the ApolloProvider. So my App.js ended up looking like this

import setupClient from './apollo';

(async () => {
  ReactDOM.render(
    <ApolloProvider client={await setupClient()}>
      <BrowserRouter>
        <App />
      </BrowserRouter>
    </ApolloProvider>,
    document.getElementById('root'),
  );
})();

Is there a better way to deal with the async propagation of persistCache?

EDIT: Actually there's a good solution in the docs

[wtrocki]

Yes. Docs are not clear about it.
@adrienharnay If you are happy to contribute the change I will be willing to merge this instantly with no issues.

[adrienharnay]

Hey, I just opened #97 to clarify the matter, and to make it clear at first glance. Feel free to edit/comment!

[wtrocki]

Goinig to make minor edit and merge. Thanks

[twelve17]

@adrienharnay how are you awaiting at the top-level in your example? Are you using the "top-level await proposal"?

https://github.com/tc39/proposal-top-level-await

[adrienharnay]

No, this is pseudo-code. Though you can wrap the code in a function and call it.

let client;

const createClient = () => {
	client = new ApolloClient({
	  cache,
	  ...
	});
};

export const getClient = () => client;

const startApp = async () => {
    await persistCache({
	  cache,
	  storage: window.localStorage,
	});
	
	// Continue setting up Apollo as usual.

    React.render(...);
};

startApp();

[twelve17]

@adrienharnay but this won't block other JS files from being read, so effectively it does not block queries from running. Is the assumption that there won't be a race condition between this file being read and another file that will run a query?

Also, it doesn't address how to export the client, which I don't think is possible, since the await for the persist makes it impossible for this code to be ready at the time of evaluation.

[adrienharnay]

Updated my gist to showcase that what you want is possible.

• this is the entry point of the app, the one that renders the tree, and it awaits for the cache to be persisted so your first render will have the cache
• you can easily export the client, but most of the time Apollo gives you HOCs, hooks or render props to access the client if you need to

[wtrocki]

I think if we talking react we probably looking for something that will not hold render but provide some nice Loading... screen until client is supplied. Keep in mind that client needs to be attached to apollo provider there.

I think good example will just have withApolloClient setup that is part of the render method and displays Loading until promise is satisfied.
Holding render is pretty much antipattern mentioned in react docs.

[wtrocki]

Btw.. I really love this discussion, because it made me realized that I was doing react wrong for long time. Getting examples correct is really important.

[twelve17]

@adrienharnay

this is the entry point of the app,

If your intention was that the context of the await is more or less akin to what is shown in the react native section of the documentation, then the top-level await comment I mentioned doesn't apply. I took the example too literally when it had been pseudo-code.

Even in your updated gist, it seems there's nothing that prevents a separate file from calling getClient() before client is set, and end up with a null value, depending on the order in which files are read. Either way, I did not intend to imply that I would want to export the Apollo Client instance if it's used within the context of a React component that would block before rendering.

More generally, where I'm going with all this is: prior to using this library, I've seen examples that encapsulate creation of the Apollo instance in a separate JS file, and since there is no asynchronous function call constraint, it can create and export the client easily:

client.js:

// client should not be "null" for any file that imports this constant.
 export const client new ApolloClient({ ... });

Then in the App.js or equivalent:

import { client } from './client';

const App = () => <ApolloProvider client={client}>....</ApolloProvider>;

IMHO, since this library adds an asynchronous function call as part of the client setup, I think it would make sense to use the approach shown in the RN example as the general example, so it emphasizes how to address the need to block before anything calls the client before the persistCache call is complete.

How does that sound? I'd be glad to do a PR to update the docs.

[twelve17]

Here's an example of a HOC that could handle this, using React hooks:

apollo-client.js

export const createClient = async () => {
  await persistCache({
    cache,
    storage: AsyncStorage,
  });
  return new ApolloClient({ .... });
};

ApolloPersistentProvider.js

import React, { useEffect, useState } from 'react';
import { createClient } from './apollo-client';
import { ApolloProvider } from '@apollo/react-common';

export const ApolloPersistentProvider = props => {
  const [client, setClient] = useState(null);

  useEffect(() => {
    if (client) return;
    // Do not return the result of this function, as useEffect functions
    // cannot return a promise. 
    // https://github.com/facebook/react/issues/14326#issuecomment-441680293
    createClient().then(newClient => {
      setClient(newClient);
    });
  }, [client]);

  return (
    !!client && (
      <ApolloProvider client={client}>{props.children}</ApolloProvider>
    )
  );
};

It doesn't show a Loading screen, though it could be modified to do so. But the amount of time that the persistCache seems to take is so short to be almost negligible (though this is just anecdotal based on my very limited testing).

[adrienharnay]

@twelve17 This code is exactly what I had before I realized I needed to asynchronously load polyfills before my app could start, so I put all my async tasks in Promise.all before the initial render. But I would say this is the approach a user would need in 90% of cases 👍

[wtrocki]

We now support synchronous API. Docs were updated

[wtrocki]

We still can provide docs for awaiting. Another element is to create separate react package later when monorepo will be done

[wtrocki]

Docs fixed