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

Resolve flow errors with ReactTestRenderer #7736

Merged
merged 5 commits into from
Sep 15, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion src/renderers/art/ReactART.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ function injectAfter(parentNode, referenceNode, node) {

// ContainerMixin for components that can hold ART nodes

const ContainerMixin = assign({}, ReactMultiChild, {
const ContainerMixin = assign({}, ReactMultiChild.prototype, {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a breaking change for ART?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't be AFAIK, it's still extending the same methods. They just exist on a class now instead of an object.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand but ART lives in its own repo. It's only here for running tests. (Yea I know it's confusing.) This means that if we had to change ART in a PR here, real ART will break with next version of React that contains this change. cc @spicyj @zpao

Copy link
Collaborator

Choose a reason for hiding this comment

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

In other words ART relies on private APIs (MultiChild), and private APIs just changed in this PR. Same for React Native until it starts using its own renderer copy (I think this hasn't happened yet).

Maybe I got it wrong but I think this is a breaking change for ART and RN and I'm not sure what we should do next.

Copy link
Contributor Author

@aweary aweary Sep 16, 2016

Choose a reason for hiding this comment

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

Thanks for explaining that, in that case it looks like this would be a breaking change. I can open PRs in RN and ART to update their use of MultiChild. Though I don't see and use of ReactMultiChild in RN

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm probably missing something obvious, but why would it? That doesn't touch ReactART or ReactMultiChild.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes flow errors introduced by #7649, so I was just wondering if we'd cut a release that contained some flow errors.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh. I don't think that's terrible if we're confident that there are no bugs. But we should definitely find a way to fix flow errors before v16 since that isn't immediately coming. Maybe we can just remove flow from ReactTestRenderer or add some suppression comments for now if ReactMultiChild needs to be a class like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue was that flow didn't understand that ReactTestComponent was inheriting mountChildren and updateChildren from ReactMultiChild with Object.assign.

One way around that is what I did initially, which is just add annotations for those methods on ReactTestComponent directly. A suppression would work too. I'd be happy to revert the class conversion and fix the flow error somehow else.

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 do that for now please. Thanks for following up. :)

/**
* Moves a child component to the supplied index.
Expand Down
2 changes: 1 addition & 1 deletion src/renderers/dom/shared/ReactDOMComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -1200,7 +1200,7 @@ ReactDOMComponent.Mixin = {
Object.assign(
ReactDOMComponent.prototype,
ReactDOMComponent.Mixin,
ReactMultiChild
ReactMultiChild.prototype
);

module.exports = ReactDOMComponent;
2 changes: 1 addition & 1 deletion src/renderers/native/ReactNativeBaseComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ ReactNativeBaseComponent.Mixin = {
*/
Object.assign(
ReactNativeBaseComponent.prototype,
ReactMultiChild,
ReactMultiChild.prototype,
ReactNativeBaseComponent.Mixin,
NativeMethodsMixin
);
Expand Down
54 changes: 27 additions & 27 deletions src/renderers/shared/stack/reconciler/ReactMultiChild.js
Original file line number Diff line number Diff line change
Expand Up @@ -169,8 +169,8 @@ if (__DEV__) {
* children. This is used by `ReactDOMComponent` to mount, update, and
* unmount child components.
*/
var ReactMultiChild = {
_reconcilerInstantiateChildren: function(nestedChildren, transaction, context) {
class ReactMultiChild {
_reconcilerInstantiateChildren(nestedChildren, transaction, context) {
if (__DEV__) {
var selfDebugID = getDebugID(this);
if (this._currentElement) {
Expand All @@ -187,9 +187,9 @@ var ReactMultiChild = {
return ReactChildReconciler.instantiateChildren(
nestedChildren, transaction, context
);
},
}

_reconcilerUpdateChildren: function(
_reconcilerUpdateChildren(
prevChildren,
nextNestedChildrenElements,
mountImages,
Expand Down Expand Up @@ -235,7 +235,7 @@ var ReactMultiChild = {
selfDebugID
);
return nextChildren;
},
}

/**
* Generates a "mount image" for each of the supplied children. In the case
Expand All @@ -245,7 +245,7 @@ var ReactMultiChild = {
* @return {array} An array of mounted representations.
* @internal
*/
mountChildren: function(nestedChildren, transaction, context) {
mountChildren(nestedChildren, transaction, context) {
var children = this._reconcilerInstantiateChildren(
nestedChildren, transaction, context
);
Expand Down Expand Up @@ -278,15 +278,15 @@ var ReactMultiChild = {
}

return mountImages;
},
}

/**
* Replaces any rendered children with a text content string.
*
* @param {string} nextContent String of content.
* @internal
*/
updateTextContent: function(nextContent) {
updateTextContent(nextContent) {
var prevChildren = this._renderedChildren;
// Remove any rendered children.
ReactChildReconciler.unmountChildren(prevChildren, false);
Expand All @@ -298,15 +298,15 @@ var ReactMultiChild = {
// Set new text content.
var updates = [makeTextContent(nextContent)];
processQueue(this, updates);
},
}

/**
* Replaces any rendered children with a markup string.
*
* @param {string} nextMarkup String of markup.
* @internal
*/
updateMarkup: function(nextMarkup) {
updateMarkup(nextMarkup) {
var prevChildren = this._renderedChildren;
// Remove any rendered children.
ReactChildReconciler.unmountChildren(prevChildren, false);
Expand All @@ -317,7 +317,7 @@ var ReactMultiChild = {
}
var updates = [makeSetMarkup(nextMarkup)];
processQueue(this, updates);
},
}

/**
* Updates the rendered children with new children.
Expand All @@ -326,18 +326,18 @@ var ReactMultiChild = {
* @param {ReactReconcileTransaction} transaction
* @internal
*/
updateChildren: function(nextNestedChildrenElements, transaction, context) {
updateChildren(nextNestedChildrenElements, transaction, context) {
// Hook used by React ART
this._updateChildren(nextNestedChildrenElements, transaction, context);
},
}

/**
* @param {?object} nextNestedChildrenElements Nested child element maps.
* @param {ReactReconcileTransaction} transaction
* @final
* @protected
*/
_updateChildren: function(nextNestedChildrenElements, transaction, context) {
_updateChildren(nextNestedChildrenElements, transaction, context) {
var prevChildren = this._renderedChildren;
var removedNodes = {};
var mountImages = [];
Expand Down Expand Up @@ -414,7 +414,7 @@ var ReactMultiChild = {
if (__DEV__) {
setChildrenForInstrumentation.call(this, nextChildren);
}
},
}

/**
* Unmounts all rendered children. This should be used to clean up children
Expand All @@ -423,11 +423,11 @@ var ReactMultiChild = {
*
* @internal
*/
unmountChildren: function(safely) {
unmountChildren(safely) {
var renderedChildren = this._renderedChildren;
ReactChildReconciler.unmountChildren(renderedChildren, safely);
this._renderedChildren = null;
},
}

/**
* Moves a child component to the supplied index.
Expand All @@ -437,14 +437,14 @@ var ReactMultiChild = {
* @param {number} lastIndex Last index visited of the siblings of `child`.
* @protected
*/
moveChild: function(child, afterNode, toIndex, lastIndex) {
moveChild(child, afterNode, toIndex, lastIndex) {
// If the index of `child` is less than `lastIndex`, then it needs to
// be moved. Otherwise, we do not need to move it because a child will be
// inserted or moved before `child`.
if (child._mountIndex < lastIndex) {
return makeMove(child, afterNode, toIndex);
}
},
}

/**
* Creates a child component.
Expand All @@ -453,19 +453,19 @@ var ReactMultiChild = {
* @param {string} mountImage Markup to insert.
* @protected
*/
createChild: function(child, afterNode, mountImage) {
createChild(child, afterNode, mountImage) {
return makeInsertMarkup(mountImage, afterNode, child._mountIndex);
},
}

/**
* Removes a child component.
*
* @param {ReactComponent} child Child to remove.
* @protected
*/
removeChild: function(child, node) {
removeChild(child, node) {
return makeRemove(child, node);
},
}

/**
* Mounts a child with the supplied name.
Expand All @@ -478,7 +478,7 @@ var ReactMultiChild = {
* @param {ReactReconcileTransaction} transaction
* @private
*/
_mountChildAtIndex: function(
_mountChildAtIndex(
child,
mountImage,
afterNode,
Expand All @@ -487,7 +487,7 @@ var ReactMultiChild = {
context) {
child._mountIndex = index;
return this.createChild(child, afterNode, mountImage);
},
}

/**
* Unmounts a rendered child.
Expand All @@ -497,11 +497,11 @@ var ReactMultiChild = {
* @param {ReactComponent} child Component to unmount.
* @private
*/
_unmountChild: function(child, node) {
_unmountChild(child, node) {
var update = this.removeChild(child, node);
child._mountIndex = null;
return update;
},
}
};

module.exports = ReactMultiChild;
3 changes: 2 additions & 1 deletion src/renderers/shared/stack/reconciler/ReactOwner.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
var invariant = require('invariant');

import type { ReactInstance } from 'ReactInstanceType';
import type { Transaction } from 'Transaction';

/**
* @param {?object} object
Expand Down Expand Up @@ -73,7 +74,7 @@ var ReactOwner = {
component: ReactInstance,
ref: string,
owner: ReactInstance,
transaction,
transaction: Transaction,
): void {
invariant(
isValidOwner(owner),
Expand Down
2 changes: 2 additions & 0 deletions src/renderers/testing/ReactTestEmptyComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
'use strict';

class ReactTestEmptyComponent {
_currentElement: null;

constructor() {
this._currentElement = null;
}
Expand Down
6 changes: 4 additions & 2 deletions src/renderers/testing/ReactTestMount.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@ var getHostComponentFromComposite = require('getHostComponentFromComposite');
var instantiateReactComponent = require('instantiateReactComponent');
var invariant = require('invariant');

type TestRendererOptions = {
createNodeMock: (element: ReactElement) => Object,
import type { ReactElement } from 'ReactElementType';

export type TestRendererOptions = {
createNodeMock: (element: ReactElement) => any,
};

var defaultTestOptions = {
Expand Down
4 changes: 3 additions & 1 deletion src/renderers/testing/ReactTestReconcileTransaction.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ var PooledClass = require('PooledClass');
var Transaction = require('Transaction');
var ReactUpdateQueue = require('ReactUpdateQueue');

import type { TestRendererOptions } from 'ReactTestMount';

/**
* Provides a `CallbackQueue` queue for collecting `onDOMReady` callbacks during
* the performing of the transaction.
Expand Down Expand Up @@ -57,7 +59,7 @@ var TRANSACTION_WRAPPERS = [ON_DOM_READY_QUEUEING];
*
* @class ReactTestReconcileTransaction
*/
function ReactTestReconcileTransaction(testOptions) {
function ReactTestReconcileTransaction(testOptions: TestRendererOptions) {
this.reinitializeTransaction();
this.testOptions = testOptions;
this.reactMountReady = CallbackQueue.getPooled(this);
Expand Down
15 changes: 10 additions & 5 deletions src/renderers/testing/ReactTestRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,13 @@ var ReactTestTextComponent = require('ReactTestTextComponent');
var ReactTestEmptyComponent = require('ReactTestEmptyComponent');

import type { ReactElement } from 'ReactElementType';
import type { ReactInstance } from 'ReactInstanceType';

type ReactTestRendererJSON = {
type: string,
props: { [propName: string]: string },
children: Array<string | ReactTestRendererJSON>,
children: null | Array<string | ReactTestRendererJSON>,
$$typeof?: any
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flow kept giving me a warning about $$typeof if I didn't mark it as optional. I think it might be because we're using Object.defineProperty?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is weird :(

}

/**
Expand All @@ -46,8 +48,13 @@ function getRenderedHostOrTextFromComponent(component) {
return component;
}

class ReactTestComponent {
class ReactTestComponent extends ReactMultiChild {
_currentElement: ReactElement;
_renderedChildren: null | Object;
Copy link
Contributor

Choose a reason for hiding this comment

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

any

_topLevelWrapper: null | ReactInstance;

constructor(element: ReactElement) {
super();
this._currentElement = element;
this._renderedChildren = null;
this._topLevelWrapper = null;
Expand Down Expand Up @@ -89,7 +96,7 @@ class ReactTestComponent {
childrenJSON.push(json);
}
}
var object = {
var object: ReactTestRendererJSON = {
type: this._currentElement.type,
props: props,
children: childrenJSON.length ? childrenJSON : null,
Expand All @@ -104,8 +111,6 @@ class ReactTestComponent {
unmountComponent(): void {}
}

Object.assign(ReactTestComponent.prototype, ReactMultiChild);

// =============================================================================

ReactUpdates.injection.injectReconcileTransaction(
Expand Down
2 changes: 2 additions & 0 deletions src/renderers/testing/ReactTestTextComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
import type { ReactText } from 'ReactTypes';

class ReactTestTextComponent {
_currentElement: ReactText;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we've been doing it consistently, but it would be good to give a bit of whitespace between types and actual JS - can you just put a newline under here (and in the other class too)

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, of course 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@zpao I added the second class usage in the codebase last week, so it's probably consistent so far :p


constructor(element: ReactText) {
this._currentElement = element;
}
Expand Down