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

[do not merge] Wait for feature element #1461

Closed
wants to merge 2 commits into from
Closed

Conversation

Timer
Copy link
Contributor

@Timer Timer commented Jan 27, 2017

Should fix CI.

@gaearon
Copy link
Contributor

gaearon commented Jan 27, 2017

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

@Timer
Copy link
Contributor Author

Timer commented Jan 27, 2017

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 Timer force-pushed the jsdom branch 2 times, most recently from 23773d4 to 1825169 Compare January 27, 2017 23:27
@Timer
Copy link
Contributor Author

Timer commented Jan 28, 2017

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

@tuchk4
Copy link
Contributor

tuchk4 commented Jan 28, 2017

@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( new Event('fixture')));
}

@Timer
Copy link
Contributor Author

Timer commented Jan 28, 2017

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

Timer commented Jan 28, 2017

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

EnoahNetzach commented Jan 28, 2017

yesterday I was fiddling around with the events idea:

class BuiltEmitter extends React.Component {
  componentDidMount() {
    document.dispatchEvent(new Event('ReactFeatureDidMount'));
  }

  render() {
    return <div>{this.props.children}</div>
  }
}

// ...

class App extends React.Component {
  // ...
  render() {
    const Feature = this.state.feature;
    return Feature ? <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
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
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
Copy link
Contributor

EnoahNetzach commented Jan 28, 2017

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
Contributor Author

Timer commented Jan 28, 2017

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. 🤔

@Timer Timer changed the title Wait for feature element [do not merge] Wait for feature element Jan 28, 2017
@Timer
Copy link
Contributor Author

Timer commented Jan 30, 2017

#1470

@Timer Timer closed this Jan 30, 2017
@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants