Use a Map of Providers to handle SSR React 16 context

[tgriesser]

A different approach than #2304, which was giving me errors when trying it out. The current approach of mutating the context objects seems like it's causing issues - this takes a different approach.

This fixes React.createContext by caching the Provider value on the context as the tree is walked, so the context can be accessed by children further down the tree. Currently requires that the ApolloProvider is the top-level, before any other React context elements.

I used the legacy context API via ApolloProvider, but could also move this all into the getDataFromTree if that seems clearer.

Closes #2291
Closes #2135
Closes #2139

Checklist:

• If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
• Make sure all of the significant new logic is covered by tests
• If this was a change that affects the external API used in GitHunt-React, update GitHunt-React and post a link to the PR in the discussion.

[emmatown]

This doesn't completely fix the issue as the incorrect value will still be used when Apollo is calling render on things. If you try the test below on the implementation here, you'll see that they fail.
While #2304 and the current implementation both mutate the context object, #2304 sets currentValue back to it's original value so it's as if it didn't do anything so I don't see how it could cause any errors.

Also, I'm curious, what errors did you get with #2304?

let defaultValue = "default";
let Context = React.createContext(defaultValue);
let providerValue = "provider";

expect(
  await renderToStringWithData(
    <React.Fragment>
      <Context.Provider value={providerValue} />
      <Context.Consumer>
        {val => {
          expect(val).toBe(defaultValue);
          return val;
        }}
      </Context.Consumer>
    </React.Fragment>
  )
).toBe(defaultValue);

expect(
  await renderToStringWithData(
    <Context.Provider value={providerValue}>
      <Context.Consumer>
        {val => {
          expect(val).toBe(providerValue);
          return val;
        }}
      </Context.Consumer>
    </Context.Provider>
  )
).toBe(providerValue);

expect(
  await renderToStringWithData(
    <Context.Consumer>
      {val => {
        expect(val).toBe(defaultValue);
        return val;
      }}
    </Context.Consumer>
  )
).toBe(defaultValue);

[tgriesser]

@mitchellhamilton I believe the issue is that getDataFromTree is asynchronous:

react-apollo/src/getDataFromTree.ts

Lines 195 to 203 in bbbb2c5

	walkTree(rootElement, rootContext, (_, instance, context, childContext) => {
	if (instance && hasFetchDataFunction(instance)) {
	const promise = instance.fetchData();
	if (isPromise<Object>(promise)) {
	promises.push({ promise, context: childContext || context, instance });
	return false;
	}
	}
	});

So in your PR, on the first pass the context is set correctly, but then is lost and reset to default when tree is walked further after the fetchData promises are resolved.

[tgriesser]

@mitchellhamilton fixed the implementation so it should cover your additional test case. Let me know if there's something it looks like I'm missing.

[emmatown]

While getDataFromTree is asynchronous, the tree walking is not.

It goes like this, walk tree to find promises -> wait for promises to resolve -> render by react.

Could you explain what cases this is solving over #2304?

[tgriesser]

I’ll work on a test case, but IIRC the issue seemed to be:

walk tree to find promises -> wait for promises to resolve -> walk tree further to find more promises -> repeat until there are no promises -> render by react

The context seemed to be lost if there are multiple levels of walking.

[emmatown]

Ohhhhhh, I totally get it now. The implementation here doesn't seem to have any fixes for the tests I showed though (I can't see any new commits) so I updated my implementation to use Maps though it doesn't use the old context and importantly it clones the map when it reaches a provider so only children have that context value.

[tgriesser]

I had added the update with your test above in this commit 137077d (and then force pushed which might have made it seem like there weren't updates). It too clones the Map so children only see the proper context.

I don't really care which gets merged, they both seem to fix the problem - I do like how yours adds a separate variable to track the "new" context rather than taking it on to the existing one. Added a note on your PR with a change that's needed to match up with the context spec.

[tgriesser]

@mitchellhamilton by the way, thanks for your work on emotion-js. I have been using it in my projects and it's awesome!

[emmatown]

I don't really mind much which gets merged either, I just added some more tests though which might be helpful.

@mitchellhamilton by the way, thanks for your work on emotion-js. I have been using it in my projects and it's awesome!

Thanks! ❤️

[elmpp]

Looking good, great news people

[elmpp]

Hello people, is there any movement here?

[hwillson]

Thanks for working on this @tgriesser! We've decided to merge in #2304, but your work here has helped move that PR forward. Thanks again!