apollo-client icon indicating copy to clipboard operation
apollo-client copied to clipboard

Not receiving the first event on subscription using the new Client Rendering API from React 18

Open andreicorpo opened this issue 2 years ago • 2 comments

Intended outcome:

When using the useSubscription hook, I want the hook to return all events visible in the Network WS Tab.

Actual outcome:

When using the new Client Rendering API from React 18, the hook now omits the first event visible on the Network WS Tab. The issue happens even when I remove all links from apollo client except for the split one.

// A HttpLink that attaches the authorization headers
const httpLink = setContext(() =>
  // By returning a function, we make sure the headers are computed at every request
  () => {
    const headers = getExtraHeaders();
    if (headers) {
      return {
        headers,
      };
    }
  }
).concat(
  new HttpLink({
    uri: GRAPHQL_MIDDLEWARE_URL,
  })
);

// A WebSocketLink that attaches the authorization headers
const wsLink = new WebSocketLink({
  uri: GRAPHQL_MIDDLEWARE_URL.replace("http", "ws"),
  // https://github.com/apollographql/subscriptions-transport-ws#constructorurl-options-websocketimpl
  options: {
    reconnect: true,
    // Setting lazy makes sure the connectParams function is called after the
    // ACCESS_TOKEN is fetched. Needed because of the queuing done by TokenFetchLink.
    lazy: true,
    // Needs to be e function in order to be lazily called
    connectionParams: getExtraHeaders,
  },
});

// The multiplexing link that selects the correct transport for the correct
// GraphQL Operation
const splitTransportLink = split(
  // IF
  isSubscription,
  // THEN
  wsLink,
  // ELSE
  httpLink
);

How to reproduce the issue:

Using the new Client Rendering API from React 18. If I use the old Client Rendering API even with React 18, it works as expected.

Versions

  System:
    OS: macOS 12.4
  Binaries:
    Node: 16.16.0 - ~/.asdf/installs/nodejs/lts/bin/node
    Yarn: 1.22.19 - /opt/homebrew/bin/yarn
    npm: 8.11.0 - ~/.asdf/plugins/nodejs/shims/npm
  Browsers:
    Chrome: 104.0.5112.79
    Safari: 15.5
  npmPackages:
    @apollo/client: ^3.6.9 => 3.6.9

andreicorpo avatar Aug 11 '22 11:08 andreicorpo

I played around with the code a little bit, and if I wrap the setResult from useSubscription.ts at line 108 in a flushSync like:

flushSync(() => {
  setResult(result);
});

It works.

andreicorpo avatar Aug 12 '22 11:08 andreicorpo

@andreicorpo 👋 thanks for letting us know you found a resolution and point to where you made this change to get it to work. I'll mention this issue to the team as having users have to modify the hook code directly probably isn't ideal DX.

jpvajda avatar Aug 17 '22 22:08 jpvajda

I'm having the same issue. I can confirm @andreicorpo patch works.

Any chance this will be prioritised soon? This is a major issue that breaks subscriptions in React 18.

c10b10 avatar Feb 01 '23 12:02 c10b10

@jpvajda, any news on this?

c10b10 avatar Mar 21 '23 17:03 c10b10

Hi, @c10b10 👋 I've just assigned myself and will be taking a look - thanks for your patience!

alessbell avatar Mar 21 '23 17:03 alessbell

Sounds great Alessia! Thank you!

c10b10 avatar Mar 22 '23 16:03 c10b10

@andreicorpo @c10b10 would either of you be able to provide a runnable reproduction here?

We have a CodeSandbox that mirrors a forkable GitHub repo and there is a subscriptions.tsx already set up, albeit using GraphQLWsLink.

It's also using the new client rendering API and I am not able to reproduce the issue, so there must be some steps I'm missing. Thanks!

alessbell avatar Apr 03 '23 15:04 alessbell

@alessbell I've forked and pushed a modified version of the code that replicates what we have setup in our project.

Unfortunately the connection is closed with the "Subprotocol not acceptable" error due to - I'm guessing - the implementation of the server.

Our server requires us to use the old subscriptions-transport-ws library because it doesn't support the new graphql-transport-ws protocol from graphql-ws.

I'm guessing "Subprotocol not acceptable" from the sandbox environment is caused by the server only implementing graphql-transport-ws.

In order to properly replicate the issue, I would need a server that supports the old subscriptions-transport-ws protocol.

c10b10 avatar Apr 13 '23 13:04 c10b10

Thanks @c10b10! Just to clarify, does the issue only happen with the subscriptions-transport-ws subprotocol? If that's the case and you need to fork the server too, you can find that here: https://codesandbox.io/s/graphqlwslink-demo-uifesi

alessbell avatar Apr 26 '23 14:04 alessbell

@alessbell it seems that the issues happens on both subprotocols and we couldn't replicate it with Apollo Server. We can replicate it with our server, and for the purposes of demonstrating this we've modifed the provided react-apollo-error-template to call a stripped down version of that server.

You can find the code here: https://github.com/andreicorpo/a-gql-ws/tree/main

As you can see, the useSubscription hook doesn't return the first event on the stream, even though you can clearly see it in WS tab in Developer Tools. As far as we've managed to figure out, it has to do with the length of the response. There is a threshold of turns size until which the flush happens.

Let me know if there's something else that you need in order to look into this.

c10b10 avatar Jul 13 '23 14:07 c10b10

Hi @c10b10 👋 Your reproduction was very helpful, thanks!

An important element in reproducing this bug is related to your specific WebSocket API (and explains why we don't see it in the Apollo Server example which sends a message every second): the two messages are always delivered at exactly the same timestamp (down to the ms):

CleanShot 2023-07-14 at 14 38 48

For reasons I'm still investigating, when rendering with the new React 18 root.render method, there is indeed a timing bug: useSubscription calls setResult twice but only results in a single re-render. All of the data winds up in the InMemoryCache but that missed render is definitely a problem. I'm still looking into the fix but will hopefully have an update soon, thanks!

alessbell avatar Jul 14 '23 18:07 alessbell

@c10b10 I've confirmed this is a timing issue with React 18's automatic batching: wrapping useSubscription's setResult in flushSync produces the "old" behavior by forcing a re-render for each message, even if they're delivered at the same time.

You can test out the fix by installing a snapshot release via npm i @apollo/[email protected] - just want to note that I don't have an exact timeline on when this fix will make it into a release since we're holding off on new releases until 3.8 goes out the door in the next week or so 🎉, but after that it should be in the first patch release of the 3.8.x series. Have a good weekend!

alessbell avatar Jul 14 '23 20:07 alessbell

Update: I've closed https://github.com/apollographql/apollo-client/pull/11066 with a note after considering @phryneas's comments about whether flushSync is appropriate here (not to mention tying useSubscription to react-dom via flushSync import).

Instead, I've opened a docs PR which calls out the fact that messages delivered in extremely quick succession may be batched in React 18 and an alternate approach to logging each message that is decoupled from React's render cycle using onData: https://github.com/apollographql/apollo-client/pull/11072

@c10b10 let me know if anything isn't clear here or if you have any other questions :)

alessbell avatar Jul 17 '23 15:07 alessbell

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. For general questions, we recommend using StackOverflow or our discord server.

github-actions[bot] avatar Aug 17 '23 00:08 github-actions[bot]