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

require react/lib/cloneWithProps directly instead of through addons #32

Merged
merged 1 commit into from
Apr 25, 2015

Conversation

iamdustan
Copy link
Contributor

No description provided.

@iamdustan
Copy link
Contributor Author

potentially fixes #27

@ericclemmons
Copy link
Owner

Dude, thanks @iamdustan! This works, so... I think a patch release should be fine, right?

@ericclemmons ericclemmons self-assigned this Apr 21, 2015
@iamdustan
Copy link
Contributor Author

That sounds like a patch release to me!

@BerkeleyTrue
Copy link

Reference react internal modules directly is risky as the React team has stated on multiple occasions that they will break folder directory structures without warning. Another alternative is to always pull 'react/addons' everywhere instead of just 'react'.

@iamdustan
Copy link
Contributor Author

react/addons are going away, too. This is just intentionally choosing to take on the risk that they arbitrarily change modules they export.

Frankly, this is one that is likely to deprecate soon anyway and we’ll need to change to React.cloneElement. https://facebook.github.io/react/blog/2015/03/03/react-v0.13-rc2.html#react.cloneelement We can’t now for two reasons:

  1. React.cloneElement may not preserver context (haven’t dug in, but the tests are throwing errors indicative of that when I tried itt).
  2. React.cloneElement behaves subtly different from cloneWithProps and therefore could be the cause of subtle bugs. That change makes more sense at a later date bundled up with other breaking changes, IMO.

Since react/addons is definitely going away in the future, propagating that practice through here and therefore through all consumers of this project sounds like an anti-pattern to me.

React devs have proven to be good stewards of the project and community. Although there is no guarantee that react/lib/cloneWithProps will always be there, we’re just trading one risk (through indirection) for another.

@BerkeleyTrue
Copy link

Good points. I am also using cloneWithProps in one of my projects and I am facing the same dilema.

@ericclemmons
Copy link
Owner

The code behind cloneWithProps isn't very complex at all, so, worst-case scenario, we use that equivalent code until v0.14 launches with the bug fix for React.cloneElement to maintain context.

Until then, @BerkeleyTrue's right. We either always use react/addons, or react/lib/whatever where needed.

Personally, I feel it's safer to use the latter so that projects consuming this project don't end up with multiple copies of React via Browserify (like I had happen) by avoiding references to react/addons.

@BerkeleyTrue
Copy link

I agree. I will probably end up using a mirror for cloneWithProps.

ericclemmons added a commit that referenced this pull request Apr 25, 2015
require react/lib/cloneWithProps directly instead of through addons
@ericclemmons ericclemmons merged commit 8e6a74c into ericclemmons:master Apr 25, 2015
@ericclemmons
Copy link
Owner

v1.1.4 is out with this. Thanks @iamdustan!

@BerkeleyTrue
Copy link

Looks like there is going to be changing addons for things are required directly (i.e. react/addons/cloneWithProps. That should solve this issue in a non brittle way. Maybe this will come out in next patch. facebook/react#3780

@iamdustan
Copy link
Contributor Author

Whoever made that change must be quite the handsome fellow.

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.

3 participants