3.0 has more restrictive JSX types than React allows for queries & mutations

ghost commented 5 years ago

Intended outcome:

The child renderers of mutation, query, and subscription components should accept valid react nodes (i.e. React.ReactNode as it accepted in the 2.x branch)

Actual outcome:

Types have been redefined to JSX.Element | null. This type is unnecessarily restrictive and prevents various patterns such as:

bool && <component>, arrays of children, etc. Suggest JSX.Element | null -> React.ReactNode as the child return types for all 3 ComponentOptions

How to reproduce the issue:

Version

  System:
    OS: macOS 10.14.6
  Binaries:
    Node: 10.16.0 - /usr/local/bin/node
    Yarn: 1.17.3 - ~/airlab/repos/nova/node_modules/.bin/yarn
    npm: 6.9.0 - /usr/local/bin/npm
  Browsers:
    Chrome: 76.0.3809.87
    Safari: 12.1.2
  npmPackages:
    apollo: ^2.16.3 => 2.16.3
    apollo-client: ^2.5.1 => 2.6.3
    react-apollo: ^3.0.0-beta.4 => 3.0.0-beta.4

hwillson commented 5 years ago

Thanks for reporting this @MarkKahn. This was changed to JSX.Element | null because the Query, Mutation and Subscription components were converted from being class based components to functional components. Functional components can't return ReactNode's. This is a long-standing issue, and is being tracked in:

Fixing this properly is dependent on https://github.com/microsoft/TypeScript/issues/21699 being addressed by the TS team. A PR was submitted a while back to fix this upstream, but it has been sitting there: https://github.com/microsoft/TypeScript/pull/29818

If you or anyone else has suggestions on how we can fix this now, we're definitely all ears (and would review a PR). To fix this we could consider switching back to ReactNode as you suggested, and then wrapping our internal props.children calls in a fragment like:

export function Mutation<TData = any, TVariables = OperationVariables>(
  props: MutationComponentOptions<TData, TVariables>
) {
  // ...
  return props.children ? <>props.children(runMutation, result)</> : null;
}

but that seems kinda horrible. Regardless, we're definitely open to suggestions. The only caveats are that we don't want to switch back to class based components, and whatever changes are proposed have to work with the graphql HOC as well (which wraps the Query, Mutation and Subscription components).

hwillson commented 5 years ago

I'll close this for now due to the reasons outlined in https://github.com/apollographql/react-apollo/issues/3314#issuecomment-525372325, but will happily re-open if anyone has a better solution / workaround we can implement (or until https://github.com/apollographql/react-apollo/issues/3314#issuecomment-525372325 is finalized).

apollographql / react-apollo

3.0 has more restrictive JSX types than React allows for queries & mutations #3314