Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use ReactDOM.hydrate() for hydrating a server-side component with Rea… #1013

Closed
wants to merge 1 commit into from
Closed

Use ReactDOM.hydrate() for hydrating a server-side component with Rea… #1013

wants to merge 1 commit into from

Conversation

elstgav
Copy link
Contributor

@elstgav elstgav commented Jan 11, 2018

Fixes the ReactDOM.render() deprecation warning for React v16 mentioned in #965

Calling ReactDOM.render() on a server-rendered component with React v16 will produce the following deprecation warning:

Warning: render(): Calling ReactDOM.render() to hydrate server-rendered markup will stop working in React v17. Replace the ReactDOM.render() call with ReactDOM.hydrate() if you want React to attach to the server HTML.

TODO

  • I’m unsure of where a test for this should go. Do we want a test? And if so, where does that go?
  • Should ReactOnRails.render() also be updated? I’m less clear on if it’s used for server-rendered components

This change is Reviewable

…ct v16

Fixes the ReactDOM.render() deprecation warning for React v16 mentioned in #965
@justin808
Copy link
Member

@elstgav Thanks! Do you have anybody using v16 that can help review this change?

In terms of "Should ReactOnRails.render() also be updated? I’m less clear on if it’s used for server-rendered components", it's a simple helper designed to be used from your JS code if you don't want to use the Rails Helper. Thus, it's not used for server rendering. In that case, would it require any updates?

In terms of tests, I'm open to your suggestions. This might be difficult as it's dependent on the React version used. I'm open to updating the code to the latest version of React so that we can test this properly.

 /**
   * ReactOnRails.render("HelloWorldApp", {name: "Stranger"}, 'app');
   *
   * Does this:
   *   ReactDOM.render(React.createElement(HelloWorldApp, {name: "Stranger"}),
   *     document.getElementById('app'))
   *
   * @param name Name of your registered component
   * @param props Props to pass to your component
   * @param domNodeId
   * @returns {virtualDomElement} Reference to your component's backing instance
   */
  render(name, props, domNodeId) {
    const componentObj = ComponentRegistry.get(name);
    const reactElement = createReactElement({ componentObj, props, domNodeId });

    // eslint-disable-next-line react/no-render-return-value
    return ReactDOM.render(reactElement, document.getElementById(domNodeId));
  },

@justin808
Copy link
Member

@elstgav can you please rebase on master? I merged your other PR.

@elstgav elstgav closed this Jan 11, 2018
@elstgav elstgav deleted the fix-react-render-deprecation-warning branch January 11, 2018 23:57
@elstgav
Copy link
Contributor Author

elstgav commented Jan 11, 2018

Shoot. I rebased and force-pushed, but that closed this PR 😕. Shall I open a new one?

@elstgav
Copy link
Contributor Author

elstgav commented Jan 12, 2018

And to answer your question, we’re using React 16 at my job. I haven’t tested the functionality yet, I got a bit bogged down with setting up the repo and test environment

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

Successfully merging this pull request may close these issues.

2 participants