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

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

Closed
tbrannam opened this issue Sep 2, 2019 · 3 comments
Closed

Comments

@tbrannam
Copy link

tbrannam commented Sep 2, 2019

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.

emitter.once('play', (experimentName, variantName) => {

@moretti
Copy link
Member

moretti commented Sep 2, 2019

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 (reactjs/rfcs#8) lands? 🤔

@tbrannam
Copy link
Author

tbrannam commented Sep 6, 2019

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
Copy link
Author

#10 resolves this issue

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