pushtell / react-ab-test

A/B testing React components and debug tools. Isomorphic with a simple, universal interface. Well documented and lightweight. Tested in popular browsers and Node.js. Includes helpers for Mixpanel and Segment.com.
MIT License
752 stars 112 forks source link

Children of experiment are not updating #10

Closed mdrobny closed 8 years ago

mdrobny commented 8 years ago

Is this intended that props.children in CoreExperiment are stored in state.variants object which makes them "static" and impossible to re-render when props in children change?

I have typical case in React when prop of a component inside a Variant changes but my component will never re-render so I can't use this module in my app 😢 And I am sad because it's really nice and well documented 😄

wehriam commented 8 years ago

First of all, thanks for the PR! It's incredibly helpful to have support from the community.

React components do not re-render when props change - this is typically handled with the componentWillReceiveProps lifecycle method. Along these lines, I'm not sure your PR is the best approach to resolving the issue, can you put together a jsFiddle which illustrates the problem you're experiencing?

mdrobny commented 8 years ago

Hey, I am glad to contribute. Yes, your right about re-render, my description is too coupled to my code in my app. I will prepare an example on jsFiddle to show what I mean. Meanwhile you can try to get a hint of my problem looking at the test I wrote (despite it's not pretty)

wehriam commented 8 years ago

The <SubComponent> in your test would not re-render if you changed the text property, even if it were not nested in an <Expiriment>.

mdrobny commented 8 years ago

http://jsfiddle.net/5z4z06m8/ - here is an example when component is not working with current version

http://jsfiddle.net/btdy9tum/5/ - here I used standalone.js from my branch and it works

mdrobny commented 8 years ago

Code in this PR https://github.com/pushtell/react-ab-test/pull/11 is the simplest solution - nearly no interference with existing code and usage.

The better solution would be updating state.variants object in componentWillUpdate of CoreExperiment component but this may interfere with existing code

mdrobny commented 8 years ago

@wehriam If you can write better solution for the problem I demonstrated in jsFiddle I would be happy to use it 😃

mkou commented 8 years ago

@wehriam @mdrobny I also have this issue, any updates on this ?

wehriam commented 8 years ago

Apologies @mkou I have not had a chance to address this yet. I believe a solution based on componentWillReceiveProps is the right way to go (as opposed to @mdrobny's approach in PR #11).

If you'd like to create a PR I'd be happy to work with you on it, but it may be some time before I can fully address the feature independently.

mkou commented 8 years ago

@mdrobny @wehriam here you go https://github.com/pushtell/react-ab-test/pull/13

wehriam commented 8 years ago

Resolved in #13. Thanks!