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

React 15 support #8

Closed
npbee opened this issue Apr 2, 2016 · 2 comments
Closed

React 15 support #8

npbee opened this issue Apr 2, 2016 · 2 comments

Comments

@npbee
Copy link
Contributor

npbee commented Apr 2, 2016

Hello and thanks for your work! I recently attempted to get this library working with React@15.0.0-rc.2 and ran into some issues. It looks like there are a couple of updates in v15 that may require some changes.

Returning null now returns a comment node instead of noscript

https://facebook.github.io/react/blog/#rendering-null-now-uses-comment-nodes
facebook/react@3581406

I don't think this should have any functional effect, we'd just need to update the tests, e.g.:

assertEqual(
  <GatewayProvider>
    <div>
      <section>
        <Gateway into="foo">
          Hello World
        </Gateway>
      </section>
      <GatewayDest name="foo"/>
    </div>
  </GatewayProvider>,
  // should equal
  <div>
    <section>
      <noscript/>
    </section>
    <div>Hello World</div>
  </div>
);

would now just be:

assertEqual(
  <GatewayProvider>
    <div>
      <section>
        <Gateway into="foo">
          Hello World
        </Gateway>
      </section>
      <GatewayDest name="foo"/>
    </div>
  </GatewayProvider>,
  // should equal
  <div>
    <section></section>
    <div>Hello World</div>
  </div>
);

In development, ref and key are defined on props with warnings

facebook/react#5744

This is relevant because of this line:

https://github.com/cloudflare/react-gateway/blob/master/src/Gateway.js#L36

renderIntoGatewayNode(props) {
  delete props.ref;   <----
  this.gatewayRegistry.addChild(this.props.into, props.children);
}

When in development, this cause a runtime error because the props object is frozen via Object.freeze and we're in strict mode. The Object.freeze is not new to React 15, so as far as I can tell the reason this has not failed before is because at the time of calling delete props.ref, the ref was never actually defined on props so it just kind of noop-ed and carried along. But now in React 15, ref is always defined so that it can throw the warning about trying to access it as a normal prop.

I can't quite figure out what the original intent was for deleting ref from props. Is that still needed? The tests all seem to pass without it.

I have a branch going here that I'd be happy to send along as a PR, but I wanted to get some clarification on the ref thing first. Also I'm not sure if it'd be better to wait until we get a proper 15.0 version so we're not relying on an RC version.

@jamiebuilds
Copy link
Contributor

We're going to bump all of our React versions in our projects at the same time. So I'd rather open up the version in package.json to support both 14 and 15.

Other than that I'd definitely accept anything that allows us to support both versions.

I think the delete props.ref is actually leftover from the previous version of react-gateway that was rendering a separate tree. So it can probably be safely removed.

@npbee
Copy link
Contributor Author

npbee commented Apr 2, 2016

Great, ok. I opened up a PR with a few changes. Not sure if it's exactly what you had in mind so let me know if there are any issues!

@npbee npbee mentioned this issue Apr 2, 2016
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

No branches or pull requests

2 participants