This repository was archived by the owner on Apr 13, 2023. It is now read-only.
Fix problems with importing react-dom/server in React Native. #2593
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We should just remove walkTree altogether, since it already doesn't work with the latest react and react-dom versions, and getDataFromTree no longer uses it.
benjamn
force-pushed
the
depend-on-react-dom
branch
from
33fcb00 to
000f8a3
Compare
If react-apollo depends directly on react-dom, and tries to be even a little bit strict about the version, we run the risk of duplicating the entire react-dom package, which is often one of the largest npm dependencies in React-based apps. A peer dependency won't automatically solve #2592, but it will provide an important clue to the solution.
benjamn
changed the title
benjamn
commented
Nov 17, 2018
| @@ -88,7 +88,8 @@ | |||
| }, | |||
| "peerDependencies": { | |||
| "apollo-client": "^2.3.8", | |||
| "react": "0.14.x || 15.* || ^15.0.0 || ^16.0.0", | |||
There was a problem hiding this comment.
@hwillson @jbaxleyiii Is it reasonable to stop advertising support for [email protected] at this point? That version line was last published two years ago (0.14.9). Also, this is just a peer dependency, not actually a guarantee that react-apollo works with [email protected], which we probably don't anymore…
benjamn
force-pushed
the
depend-on-react-dom
branch
from
f3d7b74 to
e6eb08c
Compare
React Native is obviously not Node.js, but it also doesn't identify itself as a browser-like environment, so importing `react-dom/server` fails due to the transitive dependency on Node's built-in `stream` module: #2592 (comment) To fix this problem, we need to avoid automatically importing `react-dom/server` in React Native (especially since `getDataFromTree` is unlikely to be used in RN apps). Since module dependencies are effectively static, detecting React Native at runtime is not an option, so we need help from the React Native bundler. Specifically, if we import the render function from `react-dom/server` in a small wrapper module, we should be able to override that module in React Native. According to the documentation of "Platform-specific extensions," it appears we can make this work using the `.native.js` file extension. Specifically, React Native is supposed to prefer `lib/*.native.js` modules to their `lib/*.js` counterparts: https://facebook.github.io/react-native/docs/platform-specific-code#platform-specific-extensions The `src/defaultRenderFunction.native.ts` module exports a default function that throws an exception, which makes `getDataFromTree` fail with a helpful error message in React Native. Calling `getMarkupFromTree` with a custom `renderFunction` may be a little more work for React Native apps, but that seems acceptable because server-side rendering is not a common use case in React Native, compared to client/server web applications.
benjamn
force-pushed
the
depend-on-react-dom
branch
from
e6eb08c to
7a0c192
Compare
benjamn
changed the title
hwillson
approved these changes
Nov 17, 2018
| @@ -186,7 +186,7 @@ describe('SSR', () => { | |||
| expect(elementCount).toEqual(2); | |||
| }); | |||
|
|
|||
| it('function stateless components with React 16.3 context', () => { | |||
| xit('function stateless components with React 16.3 context', () => { | |||
I thought this was supposed to be automatic, from reading these docs: https://facebook.github.io/react-native/docs/platform-specific-code#platform-specific-extensions Apparently not.
|
To be completely transparent, none of these techniques for pandering to React Native's special needs seems to be working. I'm at a loss for how to fix this without splitting the SSR-related functionality into a separate package (e.g. |
benjamn
added a commit
that referenced
this pull request
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).
|
Closing in favor of #2627. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After #2533,
getDataFromTreeis now implemented in terms ofrenderToStaticMarkup, which comes fromreact-dom/server, but the presence of thereact-domdependency was not enforced, as was revealed by #2592.If
react-apollodepends directly onreact-dom, and tries to be even a little bit strict about the version, we run the risk of duplicating the entirereact-dompackage, which is often one of the largest npm dependencies in React-based apps.A peer dependency won't automatically solve #2592, but it will provide an important clue to the solution, if my comment #2592 (comment) is accurate.