Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

Remove top-level react-dom/server import to fix #2592. #2627

Merged
merged 3 commits into from
Nov 28, 2018

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Nov 27, 2018

Supersedes #2593.
Should fix #2592.

By pushing the react-dom/server import down into the relevant functions that need it, we can avoid unconditionally importing that dependency tree, which helps in environments like React Native where react-dom/server either doesn't work or seems undesirable (see discussion in #2592).

Since the React Native bundler will still try to traverse the require("react-dom/server") dependencies, it's important to prune that dependency with a

"react-native": {
  "react-dom/server": false
}

section in react-apollo/package.json. Note that this does not prevent React Native apps from using getMarkupFromTree with an appropriate renderFunction; it simply prevents React Native's bundler from bundling the react-dom/server dependency just because react-apollo is imported.

Tested with both [email protected] and @0.57.7 (Expo SDKs 30 and 31).

Supersedes #2593.
Should fix #2592.

By pushing the react-dom/server import down into the relevant functions
that need it, we can avoid unconditionally importing that dependency tree,
which helps in environments like React Native where react-dom/server
either doesn't work or seems undesirable (see discussion in #2592).

Since the React Native bundler will still try to traverse the
require("react-dom/server") dependencies, it's important to prune that
dependency with a

  "react-native": {
    "react-dom/server": false
  }

section in react-apollo/package.json. Note that this does not prevent
React Native apps from using getMarkupFromTree with an appropriate
renderFunction; it simply prevents React Native's bundler from bundling
the react-dom/server dependency just because react-apollo is imported.

Tested with both [email protected] and @0.57.7 (Expo SDKs 30 and 31).
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it @benjamn - definitely🤞this takes care of it. Thanks!

@davetownsend
Copy link

Thanks for this. Fixes my issue.

Using RN v0.57.3 (no expo)

@benjamn benjamn merged commit 327edc5 into master Nov 28, 2018
@benjamn benjamn deleted the remove-react-dom-dependency branch November 28, 2018 04:14
@treyp
Copy link

treyp commented Nov 28, 2018

This also fixes the issue for me using RN 0.57.1 (no Expo). Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

react-apollo-browser.umd.js requiring react-dom/server on react native
4 participants