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

Update ReactCoroutine-test to only use public API #11338

Closed
wants to merge 1 commit into from
Closed

Update ReactCoroutine-test to only use public API #11338

wants to merge 1 commit into from

Conversation

jstejada
Copy link
Contributor

@jstejada jstejada commented Oct 23, 2017

In this case, these tests are currently testing apis that haven't been exposed
publicly yet, so there isn't currently an actually public api to be used.

For now, expose createCoroutine and createYield through ReactNoop
renderer, which is already used for testing (as per @gaearon's suggestion).
When these apis are exposed publicly, these tests can be updated using that api.

Part of #11299

In this case, these tests are currently testing apis that haven't been exposed
publicly yet, so there isn't currently an actual public api to be used.

For now, expose `createCoroutine` and `createYield` through the `ReactNoop`
renderer, which is already used for testing (as per @gaearon's suggestion).
When these apis are exposed publicly, these tests can be updated using that api.

Part of #11299
@gaearon
Copy link
Collaborator

gaearon commented Oct 23, 2017

@sebmarkbage Would you be okay with just exposing these as unstable_ from isomorphic package? Then we wouldn't need this.

@jstejada jstejada mentioned this pull request Oct 23, 2017
26 tasks
@gaearon
Copy link
Collaborator

gaearon commented Oct 25, 2017

The more I thought about this the more sense it seemed to make to extract it to a separate package instead of tacking those onto the Noop reconciler. I was in the middle of a packaging change anyway the past few days so I went ahead and did it in #11364.

So unfortunately I'm closing this. But I really appreciate that you sent this PR! It's what pushed me over the edge to actually address this :-)

@gaearon gaearon closed this Oct 25, 2017
@jstejada
Copy link
Contributor Author

ah cool, no worries! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants