Skip to content

[do not merge] Wait for feature element - #1461

Closed
Timer wants to merge 2 commits into
react:masterfrom
Timer:jsdom
Closed

[do not merge] Wait for feature element#1461
Timer wants to merge 2 commits into
react:masterfrom
Timer:jsdom

Conversation

@Timer

Copy link
Copy Markdown
Contributor

Should fix CI.

@gaearon

Copy link
Copy Markdown
Contributor

Interesting: it seems to fail for a different reason now.

@Timer

Copy link
Copy Markdown
ContributorAuthor

Interesting indeed ... perhaps react is mounting div and then child components on different ticks -- which would make sense for performance reasons, and we're grabbing it too soon now (10ms vs 100ms).

@Timer
Timerforce-pushed the jsdom branch 2 times, most recently from 23773d4 to 1825169CompareJanuary 27, 2017 23:27
@Timer

Copy link
Copy Markdown
ContributorAuthor

#1274 causes the annoying errors in console. There is a bug in sockjs-client that was fixed in the release we downgraded from.

@tuchk4

tuchk4 commented Jan 28, 2017

Copy link
Copy Markdown
Contributor

@Timer Maybe dispatch event at setState callback? Seems the result is same but less files are changed.

fixtures/kitchensink/integration/initDOM.js:

setFeature(feature){this.setState({ feature },()=>window.dispatchEvent(newEvent('fixture')));}

@Timer

Timer commented Jan 28, 2017

Copy link
Copy Markdown
ContributorAuthor

@tuchk4 I'm not sure if that works because the component that is set to the state sets its own state on mount.

initDOM will set state to the feature, then feature will render -- after its first render it calls set state again, however, I believe initDOM's set state will be called after the first render, not the re-render caused by set state in componentDidMount in the feature.

@Timer

Timer commented Jan 28, 2017

Copy link
Copy Markdown
ContributorAuthor

Not everything ends up firing the event -- not sure if this is by test code design or jsdom flaw... Need to investigate more, what do you think, @EnoahNetzach?

@EnoahNetzach

EnoahNetzach commented Jan 28, 2017

Copy link
Copy Markdown
Contributor

yesterday I was fiddling around with the events idea:

classBuiltEmitterextendsReact.Component{componentDidMount(){document.dispatchEvent(newEvent('ReactFeatureDidMount'));}render(){return<div>{this.props.children}</div>}}// ...classAppextendsReact.Component{// ...render(){constFeature=this.state.feature;returnFeature ? <BuiltEmitter><Feature/></BuiltEmitter> : null;}}

but I wasn't able to produce a consistent output between the E2E_FILE and E2E_URL versions after the eject.

Today I'm dedicated to investigate this, but if this PR's solution is sound, I'd say to accept it.

@EnoahNetzach

Copy link
Copy Markdown
Contributor

@Timer here the results EnoahNetzach/e2e-jsdom-fix.

TL;DR; it works.

It's not completely clear to me why in E2E_FILE I have to listen to window's load event and ReactFeatureDidMount is not even dispatched; while in E2E_URL the ReactFeatureDidMount event is correctly dispatched.

@EnoahNetzach

Copy link
Copy Markdown
Contributor

...aaaand I spoke too early.

Just to validation, using the componentDidMount should assure all children are mounted, ergo the <div id="feature-*"> is present, isn't it?

@EnoahNetzach

EnoahNetzach commented Jan 28, 2017

Copy link
Copy Markdown
Contributor

no, ok, now the problem (I don't have while running the suite locally) appears to be that promises (and await/async) are resolved too late.
It makes sense.

@Timer

Copy link
Copy Markdown
ContributorAuthor

It's not completely clear to me why in E2E_FILE I have to listen to window's load event and ReactFeatureDidMount is not even dispatched; while in E2E_URL the ReactFeatureDidMount event is correctly dispatched.

I'm running into the same problem ... puzzling. 🤔

@TimerTimer changed the title Wait for feature element[do not merge] Wait for feature elementJan 28, 2017
@tuchk4tuchk4 mentioned this pull request Jan 30, 2017
@Timer

Copy link
Copy Markdown
ContributorAuthor

#1470

@TimerTimer closed this Jan 30, 2017
@locklockBot locked and limited conversation to collaborators Jan 21, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Timer@gaearon@tuchk4@EnoahNetzach@facebook-github-bot