From 3ad0aa4ae17cbf5e5642c819dc8c5b71a6cfa203 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 11 Apr 2016 13:18:53 +0100 Subject: [PATCH 1/2] Track mounted instances with ReactDebugInstanceMap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a follow-up to #6389, also extracted from #6046. In #6046, we added a new API for associating internal instances with debug-time IDs. Third-party tools such as React DevTools and ReactPerf would use those IDs to query information about the instances, rather than use the instances directly. Here, we add the integration of `ReactDebugInstanceMap` into `ReactDOM` and `ReactDOMServer` rendering. The instances are registered during instantiation because we need to know their IDs right away: some events, such as “we are calling the constructor”, may fire before the instance is mounted. Currently we unregister instances when they get unmounted but in the future it will be possible to separate unmounting from unregistration, in case we ever want to support reparenting. One notable change here is that we now call `unmountComponent()` for server rendering in `__DEV__`. Traditionally this has not been necessary, as server rendering code path in mounting doesn’t allocate any resources it needs to release. However, now that we track every component instantiation, we also want to remove this information, so this requires a real unmounting pass on the server. Nevertheless, since this change is only relevant for `__DEV__`, it does not affect the production performance. Some existing code in `unmountComponent()` used to rely on the fact that it’s only called on the client. To mitigate this, I added a flag called `hasReactMountReady` to the transactions. It indicates whether `getReactMountReady()` is a real queue or a no-op. This way, component can tell, for example, whether it needs to run `componentWillUnmount()`, or whether it needs to ensure the node is allowed to be unmounted, such as in case of ``. One concern I have is that conditionally running `unmountComponent()` for server rendering can make it easy to introduce accidental memory leaks, as it will always run in the test environment but not in production. An alternative option would be to add a separate method called `releaseComponent()` that serves as `unmountComponent()` during server rendering only. This way relying on `unmountComponent()` deallocating a resource will be easy to spot because `unmountComponent()` still won’t run in tests for server rendering. Another concern is that we’re finally using `ReactDebugInstanceMap` in the code which means we now have a dependency on WeakMap being available in `__DEV__` builds. If this is bad, we should do something about it. --- src/isomorphic/ReactDebugInstanceMap.js | 4 + src/isomorphic/ReactDebugTool.js | 6 + .../__tests__/ReactDebugInstanceMap-test.js | 117 ++++++++++++++++++ src/renderers/dom/client/ReactMount.js | 6 +- .../dom/client/ReactReconcileTransaction.js | 1 + .../dom/server/ReactServerRendering.js | 10 +- .../server/ReactServerRenderingTransaction.js | 1 + src/renderers/dom/shared/ReactDOMComponent.js | 12 +- .../shared/reconciler/ReactChildReconciler.js | 8 +- .../reconciler/ReactCompositeComponent.js | 22 ++-- .../shared/reconciler/ReactMultiChild.js | 12 +- .../shared/reconciler/ReactReconciler.js | 8 +- .../reconciler/ReactSimpleEmptyComponent.js | 4 +- .../reconciler/instantiateReactComponent.js | 5 + src/test/ReactTestUtils.js | 8 +- 15 files changed, 190 insertions(+), 34 deletions(-) diff --git a/src/isomorphic/ReactDebugInstanceMap.js b/src/isomorphic/ReactDebugInstanceMap.js index 50dddf4ad0e..0bbce483e33 100644 --- a/src/isomorphic/ReactDebugInstanceMap.js +++ b/src/isomorphic/ReactDebugInstanceMap.js @@ -86,15 +86,18 @@ var ReactDebugInstanceMap = { } return getIDForInstance(internalInstance); }, + getInstanceByID(instanceID) { return getInstanceByID(instanceID); }, + isRegisteredInstance(internalInstance) { if (!checkValidInstance(internalInstance)) { return false; } return isRegisteredInstance(internalInstance); }, + registerInstance(internalInstance) { if (!checkValidInstance(internalInstance)) { return; @@ -107,6 +110,7 @@ var ReactDebugInstanceMap = { ); registerInstance(internalInstance); }, + unregisterInstance(internalInstance) { if (!checkValidInstance(internalInstance)) { return; diff --git a/src/isomorphic/ReactDebugTool.js b/src/isomorphic/ReactDebugTool.js index 4d2c2a3536a..04c458647e0 100644 --- a/src/isomorphic/ReactDebugTool.js +++ b/src/isomorphic/ReactDebugTool.js @@ -11,6 +11,7 @@ 'use strict'; +var ReactDebugInstanceMap = require('ReactDebugInstanceMap'); var ReactInvalidSetStateWarningDevTool = require('ReactInvalidSetStateWarningDevTool'); var warning = require('warning'); @@ -58,6 +59,10 @@ var ReactDebugTool = { onSetState() { emitEvent('onSetState'); }, + onInstantiateComponent(internalInstance) { + ReactDebugInstanceMap.registerInstance(internalInstance); + emitEvent('onInstantiateComponent', internalInstance); + }, onMountRootComponent(internalInstance) { emitEvent('onMountRootComponent', internalInstance); }, @@ -69,6 +74,7 @@ var ReactDebugTool = { }, onUnmountComponent(internalInstance) { emitEvent('onUnmountComponent', internalInstance); + ReactDebugInstanceMap.unregisterInstance(internalInstance); }, }; diff --git a/src/isomorphic/__tests__/ReactDebugInstanceMap-test.js b/src/isomorphic/__tests__/ReactDebugInstanceMap-test.js index d9a063e2c64..40a86260808 100644 --- a/src/isomorphic/__tests__/ReactDebugInstanceMap-test.js +++ b/src/isomorphic/__tests__/ReactDebugInstanceMap-test.js @@ -15,12 +15,18 @@ describe('ReactDebugInstanceMap', function() { var React; var ReactDebugInstanceMap; var ReactDOM; + var ReactDOMComponentTree; + var ReactDOMServer; + var ReactInstanceMap; beforeEach(function() { jest.resetModuleRegistry(); React = require('React'); ReactDebugInstanceMap = require('ReactDebugInstanceMap'); ReactDOM = require('ReactDOM'); + ReactDOMComponentTree = require('ReactDOMComponentTree'); + ReactDOMServer = require('ReactDOMServer'); + ReactInstanceMap = require('ReactInstanceMap'); }); function createStubInstance() { @@ -170,4 +176,115 @@ describe('ReactDebugInstanceMap', function() { ); } }); + + describe('integration', () => { + describe('ReactDOM', () => { + it('registers native components', () => { + var div = document.createElement('div'); + + var spanInst; + ReactDOM.render( + { + if (span) { + spanInst = ReactDOMComponentTree.getInstanceFromNode(span); + } + }} />, + div + ); + expect(ReactDebugInstanceMap.isRegisteredInstance(spanInst)).toBe(true); + + var pInst; + ReactDOM.render( +

{ + if (p) { + pInst = ReactDOMComponentTree.getInstanceFromNode(p); + } + }} />, + div + ); + expect(ReactDebugInstanceMap.isRegisteredInstance(spanInst)).toBe(false); + expect(ReactDebugInstanceMap.isRegisteredInstance(pInst)).toBe(true); + + ReactDOM.unmountComponentAtNode(div); + expect(ReactDebugInstanceMap.isRegisteredInstance(spanInst)).toBe(false); + expect(ReactDebugInstanceMap.isRegisteredInstance(pInst)).toBe(false); + }); + + it('registers composite components', () => { + var fooInst; + var Foo = React.createClass({ + render() { + fooInst = ReactInstanceMap.get(this); + return null; + }, + }); + var barInst; + var Bar = React.createClass({ + render() { + barInst = ReactInstanceMap.get(this); + return null; + }, + }); + var div = document.createElement('div'); + + ReactDOM.render(, div); + expect(ReactDebugInstanceMap.isRegisteredInstance(fooInst)).toBe(true); + + ReactDOM.render(, div); + expect(ReactDebugInstanceMap.isRegisteredInstance(fooInst)).toBe(false); + expect(ReactDebugInstanceMap.isRegisteredInstance(barInst)).toBe(true); + + ReactDOM.unmountComponentAtNode(div); + expect(ReactDebugInstanceMap.isRegisteredInstance(fooInst)).toBe(false); + expect(ReactDebugInstanceMap.isRegisteredInstance(barInst)).toBe(false); + }); + }); + }); + + describe('ReactDOMServer', () => { + it('registers components but unregisters them in the end', () => { + var fooInst; + var Foo = React.createClass({ + render() { + fooInst = ReactInstanceMap.get(this); + expect(ReactDebugInstanceMap.isRegisteredInstance(fooInst)).toBe(true); + return null; + }, + }); + + ReactDOMServer.renderToString(); + expect(ReactDebugInstanceMap.isRegisteredInstance(fooInst)).toBe(false); + + ReactDOMServer.renderToStaticMarkup(); + expect(ReactDebugInstanceMap.isRegisteredInstance(fooInst)).toBe(false); + }); + + it('can be used together with ReactDOM', () => { + var fooInst; + var Foo = React.createClass({ + render() { + fooInst = ReactInstanceMap.get(this); + expect(ReactDebugInstanceMap.isRegisteredInstance(fooInst)).toBe(true); + return null; + }, + }); + var barInst; + var Bar = React.createClass({ + render() { + barInst = ReactInstanceMap.get(this); + expect(ReactDebugInstanceMap.isRegisteredInstance(barInst)).toBe(true); + return

{ReactDOMServer.renderToString()}
; + }, + }); + + var div = document.createElement('div'); + ReactDOM.render(, div); + expect(ReactDebugInstanceMap.isRegisteredInstance(fooInst)).toBe(false); + expect(ReactDebugInstanceMap.isRegisteredInstance(barInst)).toBe(true); + + ReactDOM.unmountComponentAtNode(div); + expect(ReactDebugInstanceMap.isRegisteredInstance(fooInst)).toBe(false); + expect(ReactDebugInstanceMap.isRegisteredInstance(barInst)).toBe(false); + }); + }); }); diff --git a/src/renderers/dom/client/ReactMount.js b/src/renderers/dom/client/ReactMount.js index 7f6f8cac6ff..a1c420fbf7d 100644 --- a/src/renderers/dom/client/ReactMount.js +++ b/src/renderers/dom/client/ReactMount.js @@ -170,7 +170,11 @@ function batchedMountComponentIntoNode( * @see {ReactMount.unmountComponentAtNode} */ function unmountComponentFromNode(instance, container, safely) { - ReactReconciler.unmountComponent(instance, safely); + var transaction = ReactUpdates.ReactReconcileTransaction.getPooled( + /* useCreateElement */ true + ); + ReactReconciler.unmountComponent(instance, transaction, safely); + ReactUpdates.ReactReconcileTransaction.release(transaction); if (container.nodeType === DOC_NODE_TYPE) { container = container.documentElement; diff --git a/src/renderers/dom/client/ReactReconcileTransaction.js b/src/renderers/dom/client/ReactReconcileTransaction.js index fedd883bf1c..7b54019ec8f 100644 --- a/src/renderers/dom/client/ReactReconcileTransaction.js +++ b/src/renderers/dom/client/ReactReconcileTransaction.js @@ -113,6 +113,7 @@ function ReactReconcileTransaction(useCreateElement) { // `ReactTextComponent` checks it in `mountComponent`.` this.renderToStaticMarkup = false; this.reactMountReady = CallbackQueue.getPooled(null); + this.hasReactMountReady = true; this.useCreateElement = useCreateElement; } diff --git a/src/renderers/dom/server/ReactServerRendering.js b/src/renderers/dom/server/ReactServerRendering.js index 5a5d604b7dd..25c45748750 100644 --- a/src/renderers/dom/server/ReactServerRendering.js +++ b/src/renderers/dom/server/ReactServerRendering.js @@ -14,6 +14,7 @@ var ReactDOMContainerInfo = require('ReactDOMContainerInfo'); var ReactDefaultBatchingStrategy = require('ReactDefaultBatchingStrategy'); var ReactElement = require('ReactElement'); var ReactMarkupChecksum = require('ReactMarkupChecksum'); +var ReactReconciler = require('ReactReconciler'); var ReactServerBatchingStrategy = require('ReactServerBatchingStrategy'); var ReactServerRenderingTransaction = require('ReactServerRenderingTransaction'); @@ -36,12 +37,19 @@ function renderToStringImpl(element, makeStaticMarkup) { return transaction.perform(function() { var componentInstance = instantiateReactComponent(element); - var markup = componentInstance.mountComponent( + var markup = ReactReconciler.mountComponent( + componentInstance, transaction, null, ReactDOMContainerInfo(), emptyObject ); + if (__DEV__) { + // We unmount in __DEV__ to clean up the ReactDebugInstanceMap + // registrations that occur during instance construction. + // We don't do this pass in prod because we don't use it there. + ReactReconciler.unmountComponent(componentInstance, transaction); + } if (!makeStaticMarkup) { markup = ReactMarkupChecksum.addChecksumToMarkup(markup); } diff --git a/src/renderers/dom/server/ReactServerRenderingTransaction.js b/src/renderers/dom/server/ReactServerRenderingTransaction.js index 176a550c599..c56aa86f347 100644 --- a/src/renderers/dom/server/ReactServerRenderingTransaction.js +++ b/src/renderers/dom/server/ReactServerRenderingTransaction.js @@ -34,6 +34,7 @@ function ReactServerRenderingTransaction(renderToStaticMarkup) { this.reinitializeTransaction(); this.renderToStaticMarkup = renderToStaticMarkup; this.useCreateElement = false; + this.hasReactMountReady = false; } var Mixin = { diff --git a/src/renderers/dom/shared/ReactDOMComponent.js b/src/renderers/dom/shared/ReactDOMComponent.js index c897cb30efc..36f23beafaa 100644 --- a/src/renderers/dom/shared/ReactDOMComponent.js +++ b/src/renderers/dom/shared/ReactDOMComponent.js @@ -991,16 +991,16 @@ ReactDOMComponent.Mixin = { if (lastChildren != null && nextChildren == null) { this.updateChildren(null, transaction, context); } else if (lastHasContentOrHtml && !nextHasContentOrHtml) { - this.updateTextContent(''); + this.updateTextContent('', transaction); } if (nextContent != null) { if (lastContent !== nextContent) { - this.updateTextContent('' + nextContent); + this.updateTextContent('' + nextContent, transaction); } } else if (nextHtml != null) { if (lastHtml !== nextHtml) { - this.updateMarkup('' + nextHtml); + this.updateMarkup('' + nextHtml, transaction); } } else if (nextChildren != null) { this.updateChildren(nextChildren, transaction, context); @@ -1017,7 +1017,7 @@ ReactDOMComponent.Mixin = { * * @internal */ - unmountComponent: function(safely) { + unmountComponent: function(transaction, safely) { switch (this._tag) { case 'iframe': case 'object': @@ -1042,7 +1042,7 @@ ReactDOMComponent.Mixin = { * management. So we just document it and throw in dangerous cases. */ invariant( - false, + !transaction.hasReactMountReady, '<%s> tried to unmount. Because of cross-browser quirks it is ' + 'impossible to unmount some top-level components (eg , ' + ', and ) reliably and efficiently. To fix this, have a ' + @@ -1053,7 +1053,7 @@ ReactDOMComponent.Mixin = { break; } - this.unmountChildren(safely); + this.unmountChildren(transaction, safely); ReactDOMComponentTree.uncacheNode(this); EventPluginHub.deleteAllListeners(this); ReactComponentBrowserEnvironment.unmountIDFromEnvironment(this._rootNodeID); diff --git a/src/renderers/shared/reconciler/ReactChildReconciler.js b/src/renderers/shared/reconciler/ReactChildReconciler.js index 634b3444c71..e35f7b2cc29 100644 --- a/src/renderers/shared/reconciler/ReactChildReconciler.js +++ b/src/renderers/shared/reconciler/ReactChildReconciler.js @@ -100,7 +100,7 @@ var ReactChildReconciler = { } else { if (prevChild) { removedNodes[name] = ReactReconciler.getNativeNode(prevChild); - ReactReconciler.unmountComponent(prevChild, false); + ReactReconciler.unmountComponent(prevChild, transaction, false); } // The child must be instantiated before it's mounted. var nextChildInstance = instantiateReactComponent(nextElement); @@ -113,7 +113,7 @@ var ReactChildReconciler = { !(nextChildren && nextChildren.hasOwnProperty(name))) { prevChild = prevChildren[name]; removedNodes[name] = ReactReconciler.getNativeNode(prevChild); - ReactReconciler.unmountComponent(prevChild, false); + ReactReconciler.unmountComponent(prevChild, transaction, false); } } }, @@ -125,11 +125,11 @@ var ReactChildReconciler = { * @param {?object} renderedChildren Previously initialized set of children. * @internal */ - unmountChildren: function(renderedChildren, safely) { + unmountChildren: function(renderedChildren, transaction, safely) { for (var name in renderedChildren) { if (renderedChildren.hasOwnProperty(name)) { var renderedChild = renderedChildren[name]; - ReactReconciler.unmountComponent(renderedChild, safely); + ReactReconciler.unmountComponent(renderedChild, transaction, safely); } } }, diff --git a/src/renderers/shared/reconciler/ReactCompositeComponent.js b/src/renderers/shared/reconciler/ReactCompositeComponent.js index 0472e1fa86e..23ba73edcad 100644 --- a/src/renderers/shared/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/reconciler/ReactCompositeComponent.js @@ -343,7 +343,7 @@ var ReactCompositeComponentMixin = { } checkpoint = transaction.checkpoint(); - this._renderedComponent.unmountComponent(true); + ReactReconciler.unmountComponent(this._renderedComponent, transaction, true); transaction.rollback(checkpoint); // Try again - we've informed the component about the error, so they can render an error message this time. @@ -395,23 +395,25 @@ var ReactCompositeComponentMixin = { * @final * @internal */ - unmountComponent: function(safely) { + unmountComponent: function(transaction, safely) { if (!this._renderedComponent) { return; } var inst = this._instance; - if (inst.componentWillUnmount) { - if (safely) { - var name = this.getName() + '.componentWillUnmount()'; - ReactErrorUtils.invokeGuardedCallback(name, inst.componentWillUnmount.bind(inst)); - } else { - inst.componentWillUnmount(); + if (transaction.hasReactMountReady) { + if (inst.componentWillUnmount) { + if (safely) { + var name = this.getName() + '.componentWillUnmount()'; + ReactErrorUtils.invokeGuardedCallback(name, inst.componentWillUnmount.bind(inst)); + } else { + inst.componentWillUnmount(); + } } } if (this._renderedComponent) { - ReactReconciler.unmountComponent(this._renderedComponent, safely); + ReactReconciler.unmountComponent(this._renderedComponent, transaction, safely); this._renderedNodeType = null; this._renderedComponent = null; this._instance = null; @@ -843,7 +845,7 @@ var ReactCompositeComponentMixin = { ); } else { var oldNativeNode = ReactReconciler.getNativeNode(prevComponentInstance); - ReactReconciler.unmountComponent(prevComponentInstance, false); + ReactReconciler.unmountComponent(prevComponentInstance, transaction, false); this._renderedNodeType = ReactNodeTypes.getType(nextRenderedElement); this._renderedComponent = this._instantiateReactComponent( diff --git a/src/renderers/shared/reconciler/ReactMultiChild.js b/src/renderers/shared/reconciler/ReactMultiChild.js index 66f38613218..65bd1832ac7 100644 --- a/src/renderers/shared/reconciler/ReactMultiChild.js +++ b/src/renderers/shared/reconciler/ReactMultiChild.js @@ -239,10 +239,10 @@ var ReactMultiChild = { * @param {string} nextContent String of content. * @internal */ - updateTextContent: function(nextContent) { + updateTextContent: function(nextContent, transaction) { var prevChildren = this._renderedChildren; // Remove any rendered children. - ReactChildReconciler.unmountChildren(prevChildren, false); + ReactChildReconciler.unmountChildren(prevChildren, transaction, false); for (var name in prevChildren) { if (prevChildren.hasOwnProperty(name)) { invariant(false, 'updateTextContent called on non-empty component.'); @@ -259,10 +259,10 @@ var ReactMultiChild = { * @param {string} nextMarkup String of markup. * @internal */ - updateMarkup: function(nextMarkup) { + updateMarkup: function(nextMarkup, transaction) { var prevChildren = this._renderedChildren; // Remove any rendered children. - ReactChildReconciler.unmountChildren(prevChildren, false); + ReactChildReconciler.unmountChildren(prevChildren, transaction, false); for (var name in prevChildren) { if (prevChildren.hasOwnProperty(name)) { invariant(false, 'updateTextContent called on non-empty component.'); @@ -366,9 +366,9 @@ var ReactMultiChild = { * * @internal */ - unmountChildren: function(safely) { + unmountChildren: function(transaction, safely) { var renderedChildren = this._renderedChildren; - ReactChildReconciler.unmountChildren(renderedChildren, safely); + ReactChildReconciler.unmountChildren(renderedChildren, transaction, safely); this._renderedChildren = null; }, diff --git a/src/renderers/shared/reconciler/ReactReconciler.js b/src/renderers/shared/reconciler/ReactReconciler.js index ae7806e4fc0..6a6b8cfca0e 100644 --- a/src/renderers/shared/reconciler/ReactReconciler.js +++ b/src/renderers/shared/reconciler/ReactReconciler.js @@ -72,9 +72,11 @@ var ReactReconciler = { * @final * @internal */ - unmountComponent: function(internalInstance, safely) { - ReactRef.detachRefs(internalInstance, internalInstance._currentElement); - internalInstance.unmountComponent(safely); + unmountComponent: function(internalInstance, transaction, safely) { + if (transaction.hasReactMountReady) { + ReactRef.detachRefs(internalInstance, internalInstance._currentElement); + } + internalInstance.unmountComponent(transaction, safely); if (__DEV__) { ReactInstrumentation.debugTool.onUnmountComponent(internalInstance); } diff --git a/src/renderers/shared/reconciler/ReactSimpleEmptyComponent.js b/src/renderers/shared/reconciler/ReactSimpleEmptyComponent.js index 0565a016441..e62042cb288 100644 --- a/src/renderers/shared/reconciler/ReactSimpleEmptyComponent.js +++ b/src/renderers/shared/reconciler/ReactSimpleEmptyComponent.js @@ -38,8 +38,8 @@ Object.assign(ReactSimpleEmptyComponent.prototype, { getNativeNode: function() { return ReactReconciler.getNativeNode(this._renderedComponent); }, - unmountComponent: function() { - ReactReconciler.unmountComponent(this._renderedComponent); + unmountComponent: function(transaction) { + ReactReconciler.unmountComponent(this._renderedComponent, transaction); this._renderedComponent = null; }, }); diff --git a/src/renderers/shared/reconciler/instantiateReactComponent.js b/src/renderers/shared/reconciler/instantiateReactComponent.js index 796e037b995..738cf9b9a24 100644 --- a/src/renderers/shared/reconciler/instantiateReactComponent.js +++ b/src/renderers/shared/reconciler/instantiateReactComponent.js @@ -13,6 +13,7 @@ var ReactCompositeComponent = require('ReactCompositeComponent'); var ReactEmptyComponent = require('ReactEmptyComponent'); +var ReactInstrumentation = require('ReactInstrumentation'); var ReactNativeComponent = require('ReactNativeComponent'); var invariant = require('invariant'); @@ -129,6 +130,10 @@ function instantiateReactComponent(node) { } } + if (__DEV__) { + ReactInstrumentation.debugTool.onInstantiateComponent(instance); + } + return instance; } diff --git a/src/test/ReactTestUtils.js b/src/test/ReactTestUtils.js index d1365b50c54..2320cec36d9 100644 --- a/src/test/ReactTestUtils.js +++ b/src/test/ReactTestUtils.js @@ -23,6 +23,7 @@ var ReactElement = require('ReactElement'); var ReactBrowserEventEmitter = require('ReactBrowserEventEmitter'); var ReactCompositeComponent = require('ReactCompositeComponent'); var ReactInstanceMap = require('ReactInstanceMap'); +var ReactInstrumentation = require('ReactInstrumentation'); var ReactUpdates = require('ReactUpdates'); var SyntheticEvent = require('SyntheticEvent'); @@ -369,6 +370,9 @@ ReactShallowRenderer.prototype.getMountedInstance = function() { var NoopInternalComponent = function(element) { this._renderedOutput = element; this._currentElement = element; + if (__DEV__) { + ReactInstrumentation.debugTool.onInstantiateComponent(this); + } }; NoopInternalComponent.prototype = { @@ -455,7 +459,9 @@ ReactShallowRenderer.prototype.getRenderOutput = function() { ReactShallowRenderer.prototype.unmount = function() { if (this._instance) { - this._instance.unmountComponent(false); + var transaction = ReactUpdates.ReactReconcileTransaction.getPooled(true); + this._instance.unmountComponent(transaction, false); + ReactUpdates.ReactReconcileTransaction.release(transaction); } }; From f99fc8077350683f841852264c7397fc0b2eec94 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 13 Apr 2016 19:36:11 +0100 Subject: [PATCH 2/2] Replace WeakMap with a dev-only _debugID instance field --- src/isomorphic/ReactDebugInstanceMap.js | 15 +++------------ .../__tests__/ReactDebugInstanceMap-test.js | 9 ++++++++- .../reconciler/instantiateReactComponent.js | 3 +++ 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/isomorphic/ReactDebugInstanceMap.js b/src/isomorphic/ReactDebugInstanceMap.js index 0bbce483e33..9a5236f693e 100644 --- a/src/isomorphic/ReactDebugInstanceMap.js +++ b/src/isomorphic/ReactDebugInstanceMap.js @@ -35,21 +35,12 @@ function checkValidInstance(internalInstance) { return isValid; } -var idCounter = 1; var instancesByIDs = {}; -var instancesToIDs; function getIDForInstance(internalInstance) { - if (!instancesToIDs) { - instancesToIDs = new WeakMap(); - } - if (instancesToIDs.has(internalInstance)) { - return instancesToIDs.get(internalInstance); - } else { - var instanceID = (idCounter++).toString(); - instancesToIDs.set(internalInstance, instanceID); - return instanceID; - } + // This is only available in __DEV__. + // Eventually we might want to use WeakMap for storing IDs. + return internalInstance._debugID; } function getInstanceByID(instanceID) { diff --git a/src/isomorphic/__tests__/ReactDebugInstanceMap-test.js b/src/isomorphic/__tests__/ReactDebugInstanceMap-test.js index 40a86260808..abb20113c22 100644 --- a/src/isomorphic/__tests__/ReactDebugInstanceMap-test.js +++ b/src/isomorphic/__tests__/ReactDebugInstanceMap-test.js @@ -19,6 +19,8 @@ describe('ReactDebugInstanceMap', function() { var ReactDOMServer; var ReactInstanceMap; + var nextDebugID = 0; + beforeEach(function() { jest.resetModuleRegistry(); React = require('React'); @@ -27,10 +29,15 @@ describe('ReactDebugInstanceMap', function() { ReactDOMComponentTree = require('ReactDOMComponentTree'); ReactDOMServer = require('ReactDOMServer'); ReactInstanceMap = require('ReactInstanceMap'); + + nextDebugID = 0; }); function createStubInstance() { - return { mountComponent: () => {} }; + return { + mountComponent: () => {}, + _debugID: (nextDebugID++).toString(), + }; } it('should register and unregister instances', function() { diff --git a/src/renderers/shared/reconciler/instantiateReactComponent.js b/src/renderers/shared/reconciler/instantiateReactComponent.js index 738cf9b9a24..d29873c1c54 100644 --- a/src/renderers/shared/reconciler/instantiateReactComponent.js +++ b/src/renderers/shared/reconciler/instantiateReactComponent.js @@ -57,6 +57,8 @@ function isInternalComponentType(type) { ); } +var nextDebugID = 0; + /** * Given a ReactNode, create an instance that will actually be mounted. * @@ -120,6 +122,7 @@ function instantiateReactComponent(node) { if (__DEV__) { instance._isOwnerNecessary = false; instance._warnedAboutRefsInRender = false; + instance._debugID = (nextDebugID++).toString(); } // Internal instances should fully constructed at this point, so they should