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 prep #9

Merged
merged 2 commits into from
Jun 20, 2016
Merged

React 15 prep #9

merged 2 commits into from
Jun 20, 2016

Conversation

npbee
Copy link
Contributor

@npbee npbee commented Apr 2, 2016

Adds in a couple of small changes to prepare for React 15 support.

  • No deleting of ref in props.
  • Helper for properly rendering an empty component depending on the loaded React version

Fixes #8

}

return <noscript />;
})();
Copy link
Contributor

Choose a reason for hiding this comment

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

easier thing to do would be:

function Empty() {
  return null;
}

<Empty/>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's much better. I'll update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it turns out that rendering null from a stateless component is only allowed in React 15 (issue). I can do something like this:

class Empty extends React.Component {
 render() {
   return null;
 }
}

But because it's a pure component class, it's getting transformed by the babel-plugin-react-pure-components back to a stateless function before the tests run. We could just force it with something like this:

class Empty extends React.Component {
  notPure() {}
  render() {
    return null;
  }
}

Not sure I love that, but it does work. What do you think?

@StevenLangbroek
Copy link

Just went through a bunch of hoops to try this out on project currently in dev, and have found no issues so far. 👍

@dandelany
Copy link

Any updates on this? Would love to update my project to React 15, and this is the only thing preventing me from doing so. Let me know if there's anything I can do to help. Thanks!

@npbee
Copy link
Contributor Author

npbee commented May 4, 2016

As far as I could tell the only real production issue in React 15 with this lib was the delete props.ref line: 17f49ca.

Trying to support both React 14 & 15 in the tests was where it got confusing, but I'm not sure it really makes sense to do that. Unless we set up the test cases to actually run multiple versions of React, we'd only ever be testing one version. It might just be easier to:

  • Make React a peerDependency with versions "^0.14.2 || ^15.0.0". This will have the added benefit of preventing multiple React bugs for people on npm3
  • Make a devDependency of React fixed at 15.0.0 or whatever version we want to test. Seems a little weird to only test one version of React but say that we support multiple versions, but I've checked around and this seems to be what other packages are doing.

I'm happy to make more updates, just not sure what direction to go. Thoughts?

@dandelany
Copy link

Thanks for the update @npbee, and yeah, the delete props.ref is the thing causing my problems as well. Re: tests, that's an interesting question - anecdotally, I've also done what you suggest (ie. fixed 15.0.x in devDependencies) in some of my own code and it's probably the easiest option for now. If there is interest in running tests against multiple React versions, maybe it deserves a new issue.

@geyang
Copy link

geyang commented May 7, 2016

Just got here after banging my head against the error for a while. Glad to see that this is moving fast! Good work everyone!

@jacobrask
Copy link

How about we start with cherry-picking 17f49ca into master since that's seems to be useless code anyway, regardless of React version?

npbee added 2 commits May 27, 2016 08:40
This is a leftover from a previous version of the library and doesn't look to be
needed.  Leaving it in place will cause a runtime error during development with
React v15 because `ref` is always defined on `props` as a "special" prop.
Trying to delete it will throw an error because props are frozen with
`Object.freeze`
Moving React to a peer dependency plus a separate dev dependency for testing seems
to be the current standard practice for React libraries.  It allows for testing
a specific version of React while still outwardly supporting multiple versions.
Also, it helps to alleviate the multiple React problems with npm 3 users.
@npbee
Copy link
Contributor Author

npbee commented May 27, 2016

Ok I tried to cleaned up a bit and do what seemed to be the minimal amount of changes to get this working:

  • Removing the ref deletion
  • Moving React to a peer dependency plus a dev dependency. This change is not without its own caveats, but seems to be the standard practice these days for React libraries. With this change, we don't have to touch any of the test code but we can still support multiple versions.

@jamiebuilds jamiebuilds merged commit b6eb4f0 into cloudflare:master Jun 20, 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

Successfully merging this pull request may close these issues.

6 participants