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.

Sign up for GitHub

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

Jump to bottom

Use _hostContainerInfo to track test renderer options #8261

Merged
merged 5 commits into from
Nov 11, 2016
Merged

Use _hostContainerInfo to track test renderer options #8261

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c00a660
Use _hostContainerInfo to track test renderer options
Nov 10, 2016
70f274f
Throw if hostContainerInfo is not populated in getPublicInstance
Nov 10, 2016
7b8366f
Linting fixes
Nov 10, 2016
a4b488f
Remove transaction from ref lifecycle code path
Nov 11, 2016
e755a3b
Remove unused argument
gaearon Nov 11, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions scripts/fiber/tests-failing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,9 @@ src/renderers/shared/stack/reconciler/__tests__/ReactUpdates-test.js
src/renderers/shared/stack/reconciler/__tests__/refs-test.js
* Should increase refs with an increase in divs

src/renderers/testing/__tests__/ReactTestRenderer-test.js
* supports updates when using refs

src/shared/utils/__tests__/traverseAllChildren-test.js
* should warn for using maps as children with owner info

Expand Down
1 change: 1 addition & 0 deletions scripts/fiber/tests-passing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1206,6 +1206,7 @@ src/renderers/testing/__tests__/ReactTestRenderer-test.js
* gives a ref to native components
* warns correctly for refs on SFCs
* allows an optional createNodeMock function
* supports unmounting when using refs
* supports error boundaries

src/shared/utils/__tests__/KeyEscapeUtils-test.js
Expand Down
10 changes: 7 additions & 3 deletions src/renderers/testing/ReactTestMount.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,14 @@ TopLevelWrapper.isReactTopLevelWrapper = true;
function mountComponentIntoNode(
componentInstance,
transaction,
hostParent,
hostContainerInfo
) {
var image = ReactReconciler.mountComponent(
componentInstance,
transaction,
null,
null,
hostContainerInfo,
emptyObject
);
componentInstance._renderedComponent._topLevelWrapper = componentInstance;
Expand All @@ -79,12 +81,14 @@ function batchedMountComponentIntoNode(
componentInstance,
options,
) {
var transaction = ReactUpdates.ReactReconcileTransaction.getPooled(options);
var transaction = ReactUpdates.ReactReconcileTransaction.getPooled(true);
var image = transaction.perform(
mountComponentIntoNode,
null,
componentInstance,
transaction,
null,
options
);
ReactUpdates.ReactReconcileTransaction.release(transaction);
return image;
Expand All @@ -96,7 +100,7 @@ var ReactTestInstance = function(component) {
ReactTestInstance.prototype.getInstance = function() {
return this._component._renderedComponent.getPublicInstance();
};
ReactTestInstance.prototype.update = function(nextElement) {
ReactTestInstance.prototype.update = function(nextElement, options) {
invariant(
this._component,
"ReactTestRenderer: .update() can't be called after unmount."
Expand Down
12 changes: 9 additions & 3 deletions src/renderers/testing/ReactTestRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,20 +53,23 @@ class ReactTestComponent {
_currentElement: ReactElement;
_renderedChildren: null | Object;
_topLevelWrapper: null | ReactInstance;
_hostContainerInfo: null | Object;

constructor(element: ReactElement) {
this._currentElement = element;
this._renderedChildren = null;
this._topLevelWrapper = null;
this._hostContainerInfo = null;
}

mountComponent(
transaction: ReactTestReconcileTransaction,
nativeParent: null | ReactTestComponent,
nativeContainerInfo: ?null,
hostContainerInfo: null | Object,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@aweary aweary Nov 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, it should always be populated here.

context: Object,
) {
var element = this._currentElement;
this._hostContainerInfo = hostContainerInfo;
// $FlowFixMe https:/facebook/flow/issues/1805
this.mountChildren(element.props.children, transaction, context);
}
Expand All @@ -83,8 +86,11 @@ class ReactTestComponent {

getPublicInstance(transaction: ReactTestReconcileTransaction): Object {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var element = this._currentElement;
var options = transaction.getTestOptions();
return options.createNodeMock(element);
var options = this._hostContainerInfo;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (options && options.createNodeMock) {
return options.createNodeMock(element);
}
return {};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@aweary aweary Nov 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

toJSON(): ReactTestRendererJSON {
Expand Down
35 changes: 35 additions & 0 deletions src/renderers/testing/__tests__/ReactTestRenderer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,41 @@ describe('ReactTestRenderer', () => {
]);
});

it('supports unmounting when using refs', () => {
class Foo extends React.Component {
render() {
return <div ref="foo"/>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: space before />

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
const inst = ReactTestRenderer.create(
<Foo/>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

{ createNodeMock: () => 'foo' }
);
expect(() => inst.unmount()).not.toThrow();
});

it('supports updates when using refs', () => {
const log = [];
const createNodeMock = element => {
log.push(element.type);
return element.type;
};
class Foo extends React.Component {
render() {
return this.props.useDiv
? <div ref="foo" />
: <span ref="foo" />;
}
}
const inst = ReactTestRenderer.create(
<Foo useDiv={true} />,
{ createNodeMock }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: no spaces in object literals, {createNodeMock}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A linting rule for this would also be great.

);
inst.update(<Foo useDiv={false} />);
// It's called with 'div' twice (mounting and unmounting)
expect(log).toEqual(['div', 'div', 'span']);
});

it('supports error boundaries', () => {
var log = [];
class Angry extends React.Component {
Expand Down