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

Use _hostContainerInfo to track test renderer options #8261

Merged
merged 5 commits into from Nov 11, 2016

Conversation

Projects
None yet
3 participants
@aweary
Member

aweary commented Nov 10, 2016

Resolves #7740

We discussed using _hostContainerInfo in #8164 to support findDOMNode, but it also helps us here. Since the transaction isn't easily available when we're updating or unmounting, tracking the test options as _hostContainerInfo simplifies things

Use _hostContainerInfo to track test renderer options
The transaction is not available when unmounting or updating the
instance, so we track it using _hostContainerInfo
Show outdated Hide outdated src/renderers/testing/ReactTestRenderer.js
@@ -83,8 +86,11 @@ class ReactTestComponent {
getPublicInstance(transaction: ReactTestReconcileTransaction): Object {

This comment has been minimized.

@gaearon

gaearon Nov 10, 2016

Member

Can we remove transaction as an argument here now? As well as not pass it wherever we currently do?

@gaearon

gaearon Nov 10, 2016

Member

Can we remove transaction as an argument here now? As well as not pass it wherever we currently do?

This comment has been minimized.

@aweary

aweary Nov 10, 2016

Member

Yupp, I'll go ahead and do that as well 👍

@aweary

aweary Nov 10, 2016

Member

Yupp, I'll go ahead and do that as well 👍

Show outdated Hide outdated src/renderers/testing/__tests__/ReactTestRenderer-test.js
it('supports unmounting when using refs', () => {
class Foo extends React.Component {
render() {
return <div ref="foo"/>;

This comment has been minimized.

@gaearon

gaearon Nov 10, 2016

Member

Nit: space before />

@gaearon

gaearon Nov 10, 2016

Member

Nit: space before />

This comment has been minimized.

@aweary

aweary Nov 10, 2016

Member

Can we add a linting rule for this? hard to remember if lint doesn't complain.

@aweary

aweary Nov 10, 2016

Member

Can we add a linting rule for this? hard to remember if lint doesn't complain.

Show outdated Hide outdated src/renderers/testing/__tests__/ReactTestRenderer-test.js
}
}
const inst = ReactTestRenderer.create(
<Foo/>,

This comment has been minimized.

@gaearon

gaearon Nov 10, 2016

Member

Same

Show outdated Hide outdated src/renderers/testing/__tests__/ReactTestRenderer-test.js
}
const inst = ReactTestRenderer.create(
<Foo useDiv={true} />,
{ createNodeMock }

This comment has been minimized.

@gaearon

gaearon Nov 10, 2016

Member

Nit: no spaces in object literals, {createNodeMock}

@gaearon

gaearon Nov 10, 2016

Member

Nit: no spaces in object literals, {createNodeMock}

This comment has been minimized.

@aweary

aweary Nov 10, 2016

Member

A linting rule for this would also be great.

@aweary

aweary Nov 10, 2016

Member

A linting rule for this would also be great.

Show outdated Hide outdated src/renderers/testing/ReactTestRenderer.js
@@ -83,8 +86,11 @@ class ReactTestComponent {
getPublicInstance(transaction: ReactTestReconcileTransaction): Object {
var element = this._currentElement;
var options = transaction.getTestOptions();
return options.createNodeMock(element);
var options = this._hostContainerInfo;

This comment has been minimized.

@gaearon

gaearon Nov 10, 2016

Member

Let's call this hostContainerInfo everywhere and get rid of options.

@gaearon

gaearon Nov 10, 2016

Member

Let's call this hostContainerInfo everywhere and get rid of options.

Show outdated Hide outdated src/renderers/testing/ReactTestRenderer.js
}
mountComponent(
transaction: ReactTestReconcileTransaction,
nativeParent: null | ReactTestComponent,
nativeContainerInfo: ?null,
hostContainerInfo: null | Object,

This comment has been minimized.

@gaearon

gaearon Nov 10, 2016

Member

Doesn't it always exist in test renderer? We probably shouldn't check for its existence below as well.

@gaearon

gaearon Nov 10, 2016

Member

Doesn't it always exist in test renderer? We probably shouldn't check for its existence below as well.

This comment has been minimized.

@aweary

aweary Nov 10, 2016

Member

True, it should always be populated here.

@aweary

aweary Nov 10, 2016

Member

True, it should always be populated here.

Show outdated Hide outdated src/renderers/testing/ReactTestRenderer.js
if (options && options.createNodeMock) {
return options.createNodeMock(element);
}
return {};

This comment has been minimized.

@gaearon

gaearon Nov 10, 2016

Member

I don't think this branch ever runs. We always use defaultTestOptions if options didn't exist, and it returns null rather than {}. I want to completely avoid conditions here.

@gaearon

gaearon Nov 10, 2016

Member

I don't think this branch ever runs. We always use defaultTestOptions if options didn't exist, and it returns null rather than {}. I want to completely avoid conditions here.

This comment has been minimized.

@aweary

aweary Nov 10, 2016

Member

This was because of flow, _hostContainerInfo is typed as null | Object, which is valid since it is null when the instance is created. The _hostContainerInfo isn't populated until the instance mounts.

While it never should be null when this method is called, flow doesn't know that. getPublicInstance is also typed as returning an Object. That's easy to change though if you want to do null | Object.

@aweary

aweary Nov 10, 2016

Member

This was because of flow, _hostContainerInfo is typed as null | Object, which is valid since it is null when the instance is created. The _hostContainerInfo isn't populated until the instance mounts.

While it never should be null when this method is called, flow doesn't know that. getPublicInstance is also typed as returning an Object. That's easy to change though if you want to do null | Object.

This comment has been minimized.

@gaearon

gaearon Nov 10, 2016

Member

Can you throw if it is null? I think this might make it clear to Flow that it's not null.

@gaearon

gaearon Nov 10, 2016

Member

Can you throw if it is null? I think this might make it clear to Flow that it's not null.

@gaearon

gaearon requested changes Nov 10, 2016 edited

Style nits, let's remove old code, and remove unnecessary conditions.

@gaearon gaearon added this to the 15.4.0 milestone Nov 10, 2016

aweary and others added some commits Nov 10, 2016

Remove transaction from ref lifecycle code path
We don't need to pass the transaction around anymore since we store the
test options on _hostContainerInfo instead
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Nov 11, 2016

Member

LGTM. Thank you!

Member

gaearon commented Nov 11, 2016

LGTM. Thank you!

@gaearon gaearon merged commit e43aaab into facebook:master Nov 11, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

zpao added a commit that referenced this pull request Nov 16, 2016

Use _hostContainerInfo to track test renderer options (#8261)
* Use _hostContainerInfo to track test renderer options

The transaction is not available when unmounting or updating the
instance, so we track it using _hostContainerInfo

* Throw if hostContainerInfo is not populated in getPublicInstance

* Linting fixes

* Remove transaction from ref lifecycle code path

We don't need to pass the transaction around anymore since we store the
test options on _hostContainerInfo instead

* Remove unused argument

(cherry picked from commit e43aaab)

acusti added a commit to brandcast/react that referenced this pull request Mar 15, 2017

Use _hostContainerInfo to track test renderer options (#8261)
* Use _hostContainerInfo to track test renderer options

The transaction is not available when unmounting or updating the
instance, so we track it using _hostContainerInfo

* Throw if hostContainerInfo is not populated in getPublicInstance

* Linting fixes

* Remove transaction from ref lifecycle code path

We don't need to pass the transaction around anymore since we store the
test options on _hostContainerInfo instead

* Remove unused argument

@renovate renovate bot referenced this pull request Feb 2, 2018

Open

Update dependency react to v16 #29

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