From 9c3e2d833d339eb885f1a1549e0043e768bdb209 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Sun, 16 Nov 2014 10:32:46 -0800 Subject: [PATCH] Wrap every DOM node in a Composite Component ...unless they already have a wrapper. Also, add tagName to every wrapper. This ensures that refs are consistent. They always look like composite components. This effectively hides the internal implementation details of real DOM components since you can no longer get a ref to one. In the future we might want to drop this wrapper and have refs refer directly to the DOM node. I currently use a hacky way of auto-wrapping inside of ReactNativeComponent so that any given string can be wrapped. Better suggestions are welcome. --- src/browser/__tests__/ReactDOM-test.js | 7 ++--- src/browser/ui/ReactDOMComponent.js | 5 +-- src/browser/ui/ReactMount.js | 8 +---- .../ui/dom/components/ReactDOMButton.js | 1 + src/browser/ui/dom/components/ReactDOMForm.js | 1 + .../ui/dom/components/ReactDOMInput.js | 1 + .../ui/dom/components/ReactDOMOption.js | 1 + .../ui/dom/components/ReactDOMSelect.js | 1 + .../ui/dom/components/ReactDOMTextarea.js | 1 + src/core/ReactComponent.js | 6 +++- src/core/ReactNativeComponent.js | 26 ++++++++++++---- src/core/__tests__/ReactComponent-test.js | 10 ++---- .../__tests__/ReactCompositeComponent-test.js | 16 +++++----- .../ReactCompositeComponentError-test.js | 4 +-- .../__tests__/ReactInstanceHandles-test.js | 4 +-- .../ReactMultiChildReconcile-test.js | 5 ++- src/test/ReactTestUtils.js | 13 +++----- src/test/reactComponentExpect.js | 31 ++++++++++++------- 18 files changed, 80 insertions(+), 61 deletions(-) diff --git a/src/browser/__tests__/ReactDOM-test.js b/src/browser/__tests__/ReactDOM-test.js index b839089d2b..64328ee179 100644 --- a/src/browser/__tests__/ReactDOM-test.js +++ b/src/browser/__tests__/ReactDOM-test.js @@ -15,7 +15,6 @@ var React = require('React'); var ReactDOM = require('ReactDOM'); -var ReactMount = require('ReactMount'); var ReactTestUtils = require('ReactTestUtils'); var div = React.createFactory('div'); @@ -53,7 +52,7 @@ describe('ReactDOM', function() { var argDiv = ReactTestUtils.renderIntoDocument( div(null, 'child') ); - var argNode = ReactMount.getNode(argDiv._rootNodeID); + var argNode = argDiv.getDOMNode(); expect(argNode.innerHTML).toBe('child'); }); @@ -61,7 +60,7 @@ describe('ReactDOM', function() { var conflictDiv = ReactTestUtils.renderIntoDocument( div({children: 'fakechild'}, 'child') ); - var conflictNode = ReactMount.getNode(conflictDiv._rootNodeID); + var conflictNode = conflictDiv.getDOMNode(); expect(conflictNode.innerHTML).toBe('child'); }); @@ -103,7 +102,7 @@ describe('ReactDOM', function() { theBird:
} }); - var root = ReactMount.getNode(myDiv._rootNodeID); + var root = myDiv.getDOMNode(); var dog = root.childNodes[0]; expect(dog.className).toBe('bigdog'); }); diff --git a/src/browser/ui/ReactDOMComponent.js b/src/browser/ui/ReactDOMComponent.js index 3f57ec4f92..71f752e9e5 100644 --- a/src/browser/ui/ReactDOMComponent.js +++ b/src/browser/ui/ReactDOMComponent.js @@ -15,7 +15,6 @@ var CSSPropertyOperations = require('CSSPropertyOperations'); var DOMProperty = require('DOMProperty'); var DOMPropertyOperations = require('DOMPropertyOperations'); -var ReactBrowserComponentMixin = require('ReactBrowserComponentMixin'); var ReactComponent = require('ReactComponent'); var ReactBrowserEventEmitter = require('ReactBrowserEventEmitter'); var ReactMount = require('ReactMount'); @@ -148,7 +147,6 @@ function validateDangerousTag(tag) { function ReactDOMComponent(tag) { validateDangerousTag(tag); this._tag = tag; - this.tagName = tag.toUpperCase(); } ReactDOMComponent.displayName = 'ReactDOMComponent'; @@ -488,8 +486,7 @@ assign( ReactDOMComponent.prototype, ReactComponent.Mixin, ReactDOMComponent.Mixin, - ReactMultiChild.Mixin, - ReactBrowserComponentMixin + ReactMultiChild.Mixin ); module.exports = ReactDOMComponent; diff --git a/src/browser/ui/ReactMount.js b/src/browser/ui/ReactMount.js index 7124d59e3f..a6f7425654 100644 --- a/src/browser/ui/ReactMount.js +++ b/src/browser/ui/ReactMount.js @@ -135,13 +135,7 @@ function getNode(id) { * @internal */ function getNodeFromInstance(instance) { - // This instance can currently be either a public or private instance since - // native nodes are still public. - var id = instance._rootNodeID; - // TODO: Once these are only public instances, remove this conditional. - if (id == null) { - id = ReactInstanceMap.get(instance)._rootNodeID; - } + var id = ReactInstanceMap.get(instance)._rootNodeID; if (ReactEmptyComponent.isNullComponentID(id)) { return null; } diff --git a/src/browser/ui/dom/components/ReactDOMButton.js b/src/browser/ui/dom/components/ReactDOMButton.js index a5fd34caf6..79206afe9f 100644 --- a/src/browser/ui/dom/components/ReactDOMButton.js +++ b/src/browser/ui/dom/components/ReactDOMButton.js @@ -39,6 +39,7 @@ var mouseListenerNames = keyMirror({ */ var ReactDOMButton = ReactClass.createClass({ displayName: 'ReactDOMButton', + tagName: 'BUTTON', mixins: [AutoFocusMixin, ReactBrowserComponentMixin], diff --git a/src/browser/ui/dom/components/ReactDOMForm.js b/src/browser/ui/dom/components/ReactDOMForm.js index 8af4a2e7b1..20c2b038cc 100644 --- a/src/browser/ui/dom/components/ReactDOMForm.js +++ b/src/browser/ui/dom/components/ReactDOMForm.js @@ -27,6 +27,7 @@ var form = ReactElement.createFactory('form'); */ var ReactDOMForm = ReactClass.createClass({ displayName: 'ReactDOMForm', + tagName: 'FORM', mixins: [ReactBrowserComponentMixin, LocalEventTrapMixin], diff --git a/src/browser/ui/dom/components/ReactDOMInput.js b/src/browser/ui/dom/components/ReactDOMInput.js index cf3758e55a..50026420a6 100644 --- a/src/browser/ui/dom/components/ReactDOMInput.js +++ b/src/browser/ui/dom/components/ReactDOMInput.js @@ -52,6 +52,7 @@ function forceUpdateIfMounted() { */ var ReactDOMInput = ReactClass.createClass({ displayName: 'ReactDOMInput', + tagName: 'INPUT', mixins: [AutoFocusMixin, LinkedValueUtils.Mixin, ReactBrowserComponentMixin], diff --git a/src/browser/ui/dom/components/ReactDOMOption.js b/src/browser/ui/dom/components/ReactDOMOption.js index 4faa788fb4..1657703e31 100644 --- a/src/browser/ui/dom/components/ReactDOMOption.js +++ b/src/browser/ui/dom/components/ReactDOMOption.js @@ -24,6 +24,7 @@ var option = ReactElement.createFactory('option'); */ var ReactDOMOption = ReactClass.createClass({ displayName: 'ReactDOMOption', + tagName: 'OPTION', mixins: [ReactBrowserComponentMixin], diff --git a/src/browser/ui/dom/components/ReactDOMSelect.js b/src/browser/ui/dom/components/ReactDOMSelect.js index 051a9759d6..e1ac3d68d9 100644 --- a/src/browser/ui/dom/components/ReactDOMSelect.js +++ b/src/browser/ui/dom/components/ReactDOMSelect.js @@ -109,6 +109,7 @@ function updateOptions(component, propValue) { */ var ReactDOMSelect = ReactClass.createClass({ displayName: 'ReactDOMSelect', + tagName: 'SELECT', mixins: [AutoFocusMixin, LinkedValueUtils.Mixin, ReactBrowserComponentMixin], diff --git a/src/browser/ui/dom/components/ReactDOMTextarea.js b/src/browser/ui/dom/components/ReactDOMTextarea.js index 765d023652..e0f7afee60 100644 --- a/src/browser/ui/dom/components/ReactDOMTextarea.js +++ b/src/browser/ui/dom/components/ReactDOMTextarea.js @@ -50,6 +50,7 @@ function forceUpdateIfMounted() { */ var ReactDOMTextarea = ReactClass.createClass({ displayName: 'ReactDOMTextarea', + tagName: 'TEXTAREA', mixins: [AutoFocusMixin, LinkedValueUtils.Mixin, ReactBrowserComponentMixin], diff --git a/src/core/ReactComponent.js b/src/core/ReactComponent.js index ffc8334408..564c7311cc 100644 --- a/src/core/ReactComponent.js +++ b/src/core/ReactComponent.js @@ -453,7 +453,11 @@ var ReactComponent = { * @internal */ getPublicInstance: function() { - return this; + invariant( + false, + 'getPublicInstance should never be called on the base class. It must ' + + 'be overriden.' + ); } } }; diff --git a/src/core/ReactNativeComponent.js b/src/core/ReactNativeComponent.js index 9f4be86474..3f03fa1c51 100644 --- a/src/core/ReactNativeComponent.js +++ b/src/core/ReactNativeComponent.js @@ -11,6 +11,9 @@ "use strict"; +var ReactClass = require('ReactClass'); +var ReactElement = require('ReactElement'); + var assign = require('Object.assign'); var invariant = require('invariant'); @@ -37,6 +40,22 @@ var ReactNativeComponentInjection = { } }; +function autoGenerateWrapperClass(type) { + return ReactClass.createClass({ + tagName: type.toUpperCase(), + render: function() { + return new ReactElement( + type, + null, + null, + null, + null, + this.props + ); + } + }); +} + /** * Create an internal class for a specific tag. * @@ -47,12 +66,7 @@ var ReactNativeComponentInjection = { function createInstanceForTag(tag, props, parentType) { var componentClass = tagToComponentClass[tag]; if (componentClass == null) { - invariant( - genericComponentClass, - 'There is no registered component for the tag %s', - tag - ); - return new genericComponentClass(tag, props); + tagToComponentClass[tag] = componentClass = autoGenerateWrapperClass(tag); } if (parentType === tag) { // Avoid recursion diff --git a/src/core/__tests__/ReactComponent-test.js b/src/core/__tests__/ReactComponent-test.js index 35e7ada062..3b09fb2f52 100644 --- a/src/core/__tests__/ReactComponent-test.js +++ b/src/core/__tests__/ReactComponent-test.js @@ -26,10 +26,6 @@ describe('ReactComponent', function() { reactComponentExpect = require('reactComponentExpect'); getMountDepth = function(instance) { - if (instance.mountComponent) { - // Native instance - return instance._mountDepth; - } return ReactInstanceMap.get(instance)._mountDepth; }; }); @@ -306,9 +302,9 @@ describe('ReactComponent', function() { expect(getMountDepth(root)).toBe(0); expect(getMountDepth(root.refs.switcher)).toBe(1); expect(getMountDepth(root.refs.switcher.refs.box)).toBe(2); - expect(getMountDepth(root.refs.switcher.refs.switcherDiv)).toBe(4); - expect(getMountDepth(root.refs.child)).toBe(5); + expect(getMountDepth(root.refs.switcher.refs.switcherDiv)).toBe(5); + expect(getMountDepth(root.refs.child)).toBe(7); expect(getMountDepth(root.refs.switcher.refs.box.refs.boxDiv)).toBe(3); - expect(getMountDepth(root.refs.child.refs.span)).toBe(6); + expect(getMountDepth(root.refs.child.refs.span)).toBe(8); }); }); diff --git a/src/core/__tests__/ReactCompositeComponent-test.js b/src/core/__tests__/ReactCompositeComponent-test.js index ef7f503a87..9c4a4c0722 100644 --- a/src/core/__tests__/ReactCompositeComponent-test.js +++ b/src/core/__tests__/ReactCompositeComponent-test.js @@ -66,8 +66,8 @@ describe('ReactCompositeComponent', function() { * reallocated again. */ ChildUpdates = React.createClass({ - getAnchorID: function() { - return this.refs.anch._rootNodeID; + getAnchor: function() { + return this.refs.anch; }, render: function() { var className = cx({'anchorClass': this.props.anchorClassOn}); @@ -182,8 +182,8 @@ describe('ReactCompositeComponent', function() { instance.setProps({renderAnchor: false}); // Clear out the anchor // rerender instance.setProps({renderAnchor: true, anchorClassOn: false}); - var anchorID = instance.getAnchorID(); - var actualDOMAnchorNode = ReactMount.getNode(anchorID); + var anchor = instance.getAnchor(); + var actualDOMAnchorNode = anchor.getDOMNode(); expect(actualDOMAnchorNode.className).toBe(''); }); @@ -779,9 +779,11 @@ describe('ReactCompositeComponent', function() { React.unmountComponentAtNode(container); expect(innerUnmounted).toBe(true); - // , , and both
elements each call - // unmountIDFromEnvironment which calls purgeID, for a total of 4. - expect(ReactMount.purgeID.callCount).toBe(4); + // , , and both
elements and their wrappers + // each call unmountIDFromEnvironment which calls purgeID, for a total of 6. + // TODO: Test the effect of this. E.g. does the node cache get repopulated + // after a getDOMNode call? + expect(ReactMount.purgeID.callCount).toBe(6); }); it('should warn when shouldComponentUpdate() returns undefined', function() { diff --git a/src/core/__tests__/ReactCompositeComponentError-test.js b/src/core/__tests__/ReactCompositeComponentError-test.js index 865b76733e..130bbf70bb 100644 --- a/src/core/__tests__/ReactCompositeComponentError-test.js +++ b/src/core/__tests__/ReactCompositeComponentError-test.js @@ -26,8 +26,8 @@ describe('ReactCompositeComponent-error', function() { } }); - var instance = ; - instance = ReactTestUtils.renderIntoDocument(instance); + new Component(); + expect(ReactErrorUtils.guard.mostRecentCall.args[1]) .toEqual('Component.someHandler'); }); diff --git a/src/core/__tests__/ReactInstanceHandles-test.js b/src/core/__tests__/ReactInstanceHandles-test.js index 0620b40d21..bf2a71def6 100644 --- a/src/core/__tests__/ReactInstanceHandles-test.js +++ b/src/core/__tests__/ReactInstanceHandles-test.js @@ -12,6 +12,7 @@ "use strict"; var React = require('React'); +var ReactInstanceMap = require('ReactInstanceMap'); var ReactTestUtils = require('ReactTestUtils'); var ReactMount = require('ReactMount'); @@ -66,8 +67,7 @@ describe('ReactInstanceHandles', function() { if (instance === null) { return ''; } - var internal = ReactTestUtils.getInternalRepresentation(instance); - return internal._rootNodeID; + return ReactInstanceMap.get(instance)._rootNodeID; } beforeEach(function() { diff --git a/src/core/__tests__/ReactMultiChildReconcile-test.js b/src/core/__tests__/ReactMultiChildReconcile-test.js index faa9d467ef..a56075fc5e 100644 --- a/src/core/__tests__/ReactMultiChildReconcile-test.js +++ b/src/core/__tests__/ReactMultiChildReconcile-test.js @@ -87,7 +87,10 @@ var FriendsStatusDisplay = React.createClass({ // TODO: Update this to a better test that doesn't rely so much on internal // implementation details. var statusDisplays = - ReactInstanceMap.get(this)._renderedComponent._renderedChildren; + ReactInstanceMap.get(this) + ._renderedComponent + ._renderedComponent + ._renderedChildren; for (name in statusDisplays) { var child = statusDisplays[name]; var isPresent = !!child; diff --git a/src/test/ReactTestUtils.js b/src/test/ReactTestUtils.js index 291b2920ea..0527174f1e 100644 --- a/src/test/ReactTestUtils.js +++ b/src/test/ReactTestUtils.js @@ -99,14 +99,6 @@ var ReactTestUtils = { (inst.constructor === type)); }, - getInternalRepresentation: function(inst) { - // TODO: Remove this duck check once we have a separate DOM/Native instance - if (typeof inst.mountComponent === 'function') { - return inst; - } - return ReactInstanceMap.get(inst); - }, - getRenderedChildOfCompositeComponent: function(inst) { if (!ReactTestUtils.isCompositeComponent(inst)) { return null; @@ -121,7 +113,10 @@ var ReactTestUtils = { } var ret = test(inst) ? [inst] : []; if (ReactTestUtils.isDOMComponent(inst)) { - var renderedChildren = inst._renderedChildren; + var internalInstance = ReactInstanceMap.get(inst); + var renderedChildren = internalInstance + ._renderedComponent + ._renderedChildren; var key; for (key in renderedChildren) { if (!renderedChildren.hasOwnProperty(key)) { diff --git a/src/test/reactComponentExpect.js b/src/test/reactComponentExpect.js index a0638996c4..d92f985d1f 100644 --- a/src/test/reactComponentExpect.js +++ b/src/test/reactComponentExpect.js @@ -12,6 +12,7 @@ "use strict"; +var ReactInstanceMap = require('ReactInstanceMap'); var ReactTestUtils = require('ReactTestUtils'); var assign = require('Object.assign'); @@ -27,14 +28,20 @@ function reactComponentExpect(instance) { expect(instance).not.toBeNull(); - this._instance = ReactTestUtils.getInternalRepresentation(instance); + var internalInstance = ReactInstanceMap.get(instance); - expect(typeof instance).toBe('object'); - expect(typeof instance.constructor).toBe('function'); - expect(ReactTestUtils.isElement(instance)).toBe(false); + expect(typeof internalInstance).toBe('object'); + expect(typeof internalInstance.constructor).toBe('function'); + expect(ReactTestUtils.isElement(internalInstance)).toBe(false); + + return new reactComponentExpectInternal(internalInstance); } -assign(reactComponentExpect.prototype, { +function reactComponentExpectInternal(internalInstance) { + this._instance = internalInstance; +} + +assign(reactComponentExpectInternal.prototype, { // Getters ------------------------------------------------------------------- /** @@ -62,7 +69,7 @@ assign(reactComponentExpect.prototype, { this.toBeCompositeComponent(); var child = this._instance._renderedComponent; // TODO: Hide ReactEmptyComponent instances here? - return new reactComponentExpect(child); + return new reactComponentExpectInternal(child); }, /** @@ -72,14 +79,15 @@ assign(reactComponentExpect.prototype, { // Currently only dom components have arrays of children, but that will // change soon. this.toBeDOMComponent(); - var renderedChildren = this._instance._renderedChildren || {}; + var renderedChildren = + this._instance._renderedComponent._renderedChildren || {}; for (var name in renderedChildren) { if (!renderedChildren.hasOwnProperty(name)) { continue; } if (renderedChildren[name]) { if (renderedChildren[name]._mountIndex === childIndex) { - return new reactComponentExpect(renderedChildren[name]); + return new reactComponentExpectInternal(renderedChildren[name]); } } } @@ -88,15 +96,16 @@ assign(reactComponentExpect.prototype, { toBeDOMComponentWithChildCount: function(n) { this.toBeDOMComponent(); - expect(this._instance._renderedChildren).toBeTruthy(); - var len = Object.keys(this._instance._renderedChildren).length; + expect(this._instance._renderedComponent._renderedChildren).toBeTruthy(); + var len = Object.keys(this._instance._renderedComponent._renderedChildren) + .length; expect(len).toBe(n); return this; }, toBeDOMComponentWithNoChildren: function() { this.toBeDOMComponent(); - expect(this._instance._renderedChildren).toBeFalsy(); + expect(this._instance._renderedComponent._renderedChildren).toBeFalsy(); return this; },