marvelapp / 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
240 stars 47 forks source link

isomorphic emitter related tests may not work well in newer react #9

Closed tbrannam closed 4 years ago

tbrannam commented 4 years ago

While evaluating changes required for 17.x - it appears that the isomorphic tests expect emitters to fire during componentWillMount. SSR typically triggers this event, but it is deprecated. The porting documentation suggests that browser implementations should transition to componentDidMount - but in an SSR environment, that is not something that is triggered. The alternative is to emit an event in the constructor, but that seems like an anti-pattern. I would suggest that these tests should be changed to not use the emitter to determine what was rendered, but instead use an inspection of the rendered DOM.

https://github.com/marvelapp/react-ab-test/blob/d2afebffa9a79a67c8fb0c701a265c8f2090d8c4/test/isomorphic/experiment.test.jsx#L52

moretti commented 4 years ago

Hi @tbrannam, thank you for opening this issue.

It's not entirely clear how SSR should trigger side-effects, I'm looking at this example: https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html#fetching-external-data and the note says:

When supporting server rendering, it’s currently necessary to provide the data synchronously – componentWillMount was often used for this purpose but the constructor can be used as a replacement. The upcoming suspense APIs will make async data fetching cleanly possible for both client and server rendering.

There's another interesting discussion about this here.

I agree that the constructor doesn't seem the right place to trigger a side-effect, perhaps the solution is to use UNSAFE_componentWillMount on the server until suspense or an alternative life-cycle method (https://github.com/reactjs/rfcs/pull/8) lands? 🤔

tbrannam commented 4 years ago

The goal of this test is to ensure that SSR is rendering the specified variant - this can be done via inspection of the rendered output - bypassing the need to check for events during SSR.

tbrannam commented 4 years ago

10 resolves this issue