feat: Implement SSR.

[avocadowastaken]

Depends on #39 and #42. Tests copied from react-apollo.

Closes #8.

[seeden]

Hi guys. please merge it :) I really need this feature

[avocadowastaken]

@trojanowski ready for review

[avocadowastaken]

This is also a good place for #15

[trojanowski]

I'd change the order here - SSR should work also for queries using the default suspense mode (at least until suspense-ready official React server renderer is available). So rather:

  if (currentResult.partial) {
    // Register request only when `ssr: true`.
    if (ssrInUse) {
      ssrManager.register(observableQuery.result());
    } else if (suspend) {
      // throw a promise - use the react suspense to wait until the data is
      // available
      throw observableQuery.result();
    }
  }

Also tests should check both suspense and non-suspense mode.

[avocadowastaken]

Suspense does not work in react-dom

[trojanowski]

Suspense does not work in react-dom

I'm not sure I understand. I changed the code to the one in the snipped above and removed suspend: false from useQuery invocations in getMarkupFromTree-test.tsx and tests passed. Or do you mean that the <Suspense /> component is not supported by the current version of react-dom-server renderer? We could workaround it by creating our custom helper component and recommending requiring to use it instead of <Suspense /> from React:

export const SuspenseSsr = (props: SuspenseProps) => {
  const ssrManager = useContext(SSRContext);
  if (ssrManager) {
    return <>{props.children}</>;
  }

  return <Suspense {...props} />;
};

[avocadowastaken]

i think it’s better to force developers pass suspend: false if they want to use ssr, otherwise they will have different behavior on ssr and client side.
suspense assures user that data is not null on render, and if ignore suspense on ssr - this can create random null pointer errors. Current approach will escalate suspended promises to react-dom and it will give good error message.

[avocadowastaken]

oops, i’m writing from phone and didn’t saw your last message.

but my previous message also covers your comment.

from my experience when you’re writing ssr app - you have to write it ssr first otherwise it would be hacky and buggy. it’s better to wait for officials suspense support from react. if someone REALLY wants to use suspense for components that also rendered on server and deal with all the problem it produeses - they can write their own suspense component

[avocadowastaken]

wouldn’t it better to name it “unstable_SuspenseSSR” then 🤔. i like the idea, i’ll try to implement it tonight

[trojanowski]

wouldn’t it better to name it “unstable_SuspenseSSR” then 🤔.

👍

i like the idea, i’ll try to implement it tonight

Thank you :)

[avocadowastaken]

@trojanowski I could't wait 🙂. So I start catching thrown promises in getMarkupFromTree and add tests for suspend: true.

[seeden]

I can not wait too 👍 thanks guys

[trojanowski]

@umidbekkarimov Looks great, thanks :) I added a few comments about testing.

[trojanowski]

The tests are actually the same for suspene / non-suspense versions. We could make it more DRY with something like that:

[true, false].forEach(suspend => {
  const prefix = suspend ? 'with' : 'without';

  describe(`${prefix} suspense`, () => {
    it('should run through all of the queries that want SSR with suspense', async () => {
      const client = createMockClient();

      await expect(
        getMarkupFromTree({
          renderFunction: renderToString,
          tree: <UserDetailsWrapper client={client} suspend={suspend} />,
        })
      ).resolves.toBe('James');
    });

    // the same for other tests
  });
});

Inline snapshots wouldn't work in that case but it shouldnt't be a problem - we were checking simple strings so we could easily replace toMatchInlineSnapshot with toBe.

[avocadowastaken]

I used describe.each instead and all tests MUST have same output so toMatchInlineSnapshot works just fine.

[trojanowski]

Good idea.

[trojanowski]

What's the purpose of using <TestErrorBoundary> here? Tests are passing also without it.

[avocadowastaken]

I wanted to test this block

react-apollo-hooks/src/getMarkupFromTree.tsx

Lines 25 to 31 in 2425cfa

	} catch (e) {
	if (!isPromiseLike(e)) {
	throw e;
	}
	ssrManager.register(e);
	}

But I guess I found the better way and forgot to remove 🤔

[avocadowastaken]

I kept using toMatchInlineSnapshot to keep it "visual" and without manual labor 🙂

[alidcast]

@trojanowski when will a release with this PR be done?

also, is server-rendering compatible with react-apollo, or will it only allow using one library?

[trojanowski]

when will a release with this PR be done?

@alidcastano I'm going to add documentation for this feature and then release it - I'll try to do it tomorrow.

also, is server-rendering compatible with react-apollo, or will it only allow using one library?

Right now it's not compatible with react-apollo, but I'm going to add a compatibility layer.

[trojanowski]

@alidcastano This PR is published as a part of release 0.3.0.

[sandiiarov]

Just curious. Was there a specific reason to wrap children into fragment?