Support the official React 16.3 Context in getDataFromTree

[marnusw]

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.

The components in the new React Context API created with React.createContext() render different to standard React components. In particular the Context.Consumer was ending traversal early in walkTree since it presents as a basic DOM element but its child is a render prop function.

The Context.Provider renders its children in a more standard way and was traversed, but the context value was not being set.

With this traversal of the official context API works as expected.

[marnusw]

The 3rd commit shows the actual change to getDataFromTree after the file was reformatted with Prettier.

The tests fail due to TypeScript errors (it works with npm run test-watch). I have attempted to fix that in the final commit, but it was unsuccessful. The new Context type was introduced in @types/react 16.3.14 which has been upgraded, but the properties on element.type are still failing even with the custom type definitions. Any help in fixing this will be appreciated.

[stubailo]

@marnusw thanks for the PR! This is an important issue. Would it be possible to modify the PR to only include code changes? Right now there are a ton of lines in the diff but it looks like a lot of them are just automated formatting.

[marnusw]

@stubailo It seems the getDataFromTree file was never reformatted with Prettier when it was introduced. I'm not sure, therefore, how I could clean up the PR as a whole unless I commit without the precommit hook checks (which I will do if that is preferred).

I tried to alleviate this problem by making my first commit formatting only, i.e. I let Prettier reformat the file and committed only those changes. The second commit adds tests and the third commit contains the only changes to the walkTree implementation.

I was hoping this breakdown would make it more practical to review the changes, but if not just let me know and I'll commit only the changes by bypassing Prettier.

The 4th commit was my attempt to fix the Typescript errors, but that was entirely unsuccessful and it should be changed to a proper fix. I couldn't figure out why it kept failing though, so I'll need some help on that please.

[stubailo]

Oh I'm sorry, I didn't think to look at the individual commits, that's a great idea!

[danb235]

Any updates on when this can be merged. It's holding up our ability to handle open graph tags in our app. 😬

[marnusw]

I have realised in the meantime that this implementation doesn't handle the default context value yet. It will be simple to add, I just need to make that update (will do soon) before this is ready to be merged. That is aside from review/approval by the maintainers.

[afenton90]

Any help you need on this let me know. I'm keen to add this as well for a project I am working on.

[hwillson]

Hi all - I started reviewing this and ran into the same typescript compilation issues mentioned in #1978 (comment). I'm looking into the cause further, and should have an update shortly. @marnusw if you're interested in adding default context support, that would be awesome - thanks!

[marnusw]

Upon further investigation the default values were working fine all along, but now there are tests to prove that as well.

[seanconnollydev]

@hwillson were you able to work through the typescript issues?

[hwillson]

@goldenshun I've been 100% side tracked due to a push to get all of the apollo-client PR's cleared out. We're almost done, so I'll be back on this shortly. Thanks!

[mike-marcacci]

Hey @marnusw - thanks so much for fixing this! (I just realized I'd silently lost SSR for the past week – time to add tests to cover that!)

Given @hwillson's mention that they'll bet tackling this really soon, would you mind fixing the merge conflicts & tests in preparation, in case that could help move this along? Thx again for the fix!

[amannn]

In addition to context, I've seen that getDerivedStateFromProps is missing to support React 16.3. I've made a PR for that.

[mike-marcacci]

Hey @marnusw – instead of leaving a review, I submitted a PR to your fork: marnusw#2

This fixes the typing and merge conflicts, and also adds a fallback to _defaultValue as you mentioned above. I any-typed the "private" properties which will probably never be accepted to @types/react, but if someone wants to add these, send a PR here.

[marnusw]

Green except for the Preact tests where React.createContext does not exist.

Thank you @mike-marcacci for fixing the Typescript errors. Casting the context internals to any is a very simple solution. In the end it's not necessary to export any additional types. I also found that an explicit fallback to the _defaultValue isn't necessary since the context constructor sets it on the _currentValue to the same already. There are passing tests for both cases now.

@hwillson what will be the best strategy for skipping the related tests in the Preact test suite? It looks like there is a PR for it on the Preact project, so this will hopefully be only temporary. Would something as simple as returning early from the test when typeof React.createContext === 'undefined' work or is there a better way?

[marnusw]

To skip the context tests in Preact I have added

if (!React.createContext) {
  return;
}

Doing it this way means the tests will be included again as soon as Preact supports the new context API without needing any changes. Then these checks could be removed when convenient.

[seanconnollydev]

@hwillson Is this ready to go?

[hwillson]

Hi all - this is looking great! I'll do a final review shortly, but I'm planning on getting this into next Tuesday's (June 19th, 2018) release. Thanks everyone!

[hwillson]

Thanks you very much for your work on this @marnusw and @mike-marcacci! We should be all set here, so LGTM!