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

Implement createNodeMock for ReactTestRenderer #7649

Merged
merged 15 commits into from
Sep 13, 2016
27 changes: 21 additions & 6 deletions src/renderers/testing/ReactTestMount.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ var getHostComponentFromComposite = require('getHostComponentFromComposite');
var instantiateReactComponent = require('instantiateReactComponent');
var invariant = require('invariant');

type TestRendererMockConfig = {
getMockRef: (element: ReactElement) => Object,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how this would be typed, is there a better way to type an object that looks like a DOM node?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks fine to me.

};

/**
* Temporary (?) hack so that we can store all top-level pending updates on
* composites instead of having to worry about different types of components
Expand All @@ -45,7 +49,9 @@ TopLevelWrapper.isReactTopLevelWrapper = true;
*/
function mountComponentIntoNode(
componentInstance,
transaction) {
transaction,
mockConfig,
) {
var image = ReactReconciler.mountComponent(
componentInstance,
transaction,
Expand All @@ -54,6 +60,9 @@ function mountComponentIntoNode(
emptyObject
);
componentInstance._renderedComponent._topLevelWrapper = componentInstance;
if (mockConfig) {
componentInstance._renderedComponent._mockConfig = mockConfig;
}
return image;
}

Expand All @@ -65,13 +74,16 @@ function mountComponentIntoNode(
* @param {number} containerTag container element to mount into.
*/
function batchedMountComponentIntoNode(
componentInstance) {
componentInstance,
mockConfig,
) {
var transaction = ReactUpdates.ReactReconcileTransaction.getPooled();
var image = transaction.perform(
mountComponentIntoNode,
null,
componentInstance,
transaction
transaction,
mockConfig,
);
ReactUpdates.ReactReconcileTransaction.release(transaction);
return image;
Expand Down Expand Up @@ -135,22 +147,25 @@ ReactTestInstance.prototype.toJSON = function() {
var ReactTestMount = {

render: function(
nextElement: ReactElement<any>
nextElement: ReactElement<any>,
mockConfig: TestRendererMockConfig,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably ?TestRendererMockConfig, it’s optional.

): ReactTestInstance {
var nextWrappedElement = React.createElement(
TopLevelWrapper,
{ child: nextElement }
{ child: nextElement },
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 need for extra spacing.

);

var instance = instantiateReactComponent(nextWrappedElement, false);

// The initial render is synchronous but any updates that happen during
// rendering, in componentWillMount or componentDidMount, will be batched
// according to the current batching strategy.
//
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: extra line


ReactUpdates.batchedUpdates(
batchedMountComponentIntoNode,
instance
instance,
mockConfig,
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 pass default here instead, as this is the first place where it appears?

);
return new ReactTestInstance(instance);
},
Expand Down
13 changes: 9 additions & 4 deletions src/renderers/testing/ReactTestRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ var ReactTestComponent = function(element) {
this._currentElement = element;
this._renderedChildren = null;
this._topLevelWrapper = null;
this._mockConfig = null;
};
ReactTestComponent.prototype.mountComponent = function(
transaction,
Expand All @@ -62,9 +63,13 @@ ReactTestComponent.prototype.receiveComponent = function(
};
ReactTestComponent.prototype.getHostNode = function() {};
ReactTestComponent.prototype.getPublicInstance = function() {
// I can't say this makes a ton of sense but it seems better than throwing.
// Maybe we'll revise later if someone has a good use case.
return null;
if (this._mockConfig) {
var { getMockRef } = this._mockConfig;
if (getMockRef) {
return getMockRef(this._currentElement);
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 make a default mockConfig implementation and always assume mockConfig exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we have a default getMockRef implementation as well and just return this._mockConfig.getMockRef(this._currentElement)? Or just keep that check

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yea, default impl is what I meant.

}
}
return {};
};
ReactTestComponent.prototype.unmountComponent = function() {};
ReactTestComponent.prototype.toJSON = function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

Expand Down Expand Up @@ -134,9 +139,9 @@ ReactComponentEnvironment.injection.injectEnvironment({
replaceNodeWithMarkup: function() {},
});


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 remove this newline

var ReactTestRenderer = {
create: ReactTestMount.render,

/* eslint-disable camelcase */
Copy link
Collaborator

Choose a reason for hiding this comment

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

And keep this one, so the diff is smaller. In general better not to formatting changes to code you didn’t directly touch unless it’s horribly formatted

unstable_batchedUpdates: ReactUpdates.batchedUpdates,
/* eslint-enable camelcase */
Expand Down
19 changes: 18 additions & 1 deletion src/renderers/testing/__tests__/ReactTestRenderer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,24 @@ describe('ReactTestRenderer', function() {
it('gives a ref to native components', function() {
var log = [];
ReactTestRenderer.create(<div ref={(r) => log.push(r)} />);
expect(log).toEqual([null]);
expect(log).toEqual([{}]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Per @spicyj, let’s keep null the default one.

});

it('allows an optional getMockRef function', function() {
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 also test that we receive the correct element.

var mockDOMInstance = { focus: () => {} };
var log = [];
ReactTestRenderer.create(
<div ref={(r) => log.push(r)} />,
{
getMockRef: instance => {
return mockDOMInstance;
},
}
);
ReactTestRenderer.create(
<div ref={(r) => log.push(r)} />,
);
expect(log).toEqual([mockDOMInstance, {}]);
});

it('supports error boundaries', function() {
Expand Down