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

Use ES6 Map in ReactComponentTreeHook if available #7491

Merged
merged 3 commits into from
Aug 13, 2016
Merged

Use ES6 Map in ReactComponentTreeHook if available #7491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1d0c762
Use ES6 Map in ReactComponentTreeHook if available
gaearon Aug 13, 2016
e5de569
Make getRootIDs fast again
gaearon Aug 13, 2016
dd495fa
Only use native Map
gaearon Aug 13, 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
85 changes: 71 additions & 14 deletions src/isomorphic/hooks/ReactComponentTreeHook.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,46 @@ var ReactCurrentOwner = require('ReactCurrentOwner');
var invariant = require('invariant');
var warning = require('warning');

var itemByKey = {};
var unmountedIDs = {};
var rootIDs = {};
function isNative(fn) {
// Based on isNative() from Lodash
var funcToString = Function.prototype.toString;
var hasOwnProperty = Object.prototype.hasOwnProperty;
var reIsNative = RegExp('^' + funcToString
// Take an example native function source for comparison
.call(hasOwnProperty)
// Strip regex characters so we can use it for regex
.replace(/[\\^$.*+?()[\]{}|]/g, '\\$&')
// Remove hasOwnProperty from the template to make it generic
.replace(
/hasOwnProperty|(function).*?(?=\\\()| for .+?(?=\\\])/g,
'$1.*?'
) + '$'
);
try {
var source = funcToString.call(fn);
return reIsNative.test(source);
} catch (err) {
return false;
}
}

var itemMap;
var itemByKey;

var canUseMap = (
typeof Array.from === 'function' &&
typeof Map === 'function' &&
isNative(Map)
);

if (canUseMap) {
itemMap = new Map();
} else {
itemByKey = {};
}

var unmountedIDs = [];
var rootIDs = [];

// Use non-numeric keys to prevent V8 performance issues:
// https://github.com/facebook/react/pull/7232
Expand All @@ -30,25 +67,37 @@ function getIDFromKey(key) {
}

function get(id) {
if (canUseMap) {
return itemMap.get(id);
}
var key = getKeyFromID(id);
return itemByKey[key];
}

function remove(id) {
if (canUseMap) {
itemMap.delete(id);
return;
}
var key = getKeyFromID(id);
delete itemByKey[key];
}

function create(id, element, parentID) {
var key = getKeyFromID(id);
itemByKey[key] = {
var item = {
element,
parentID,
text: null,
childIDs: [],
isMounted: false,
updateCount: 0,
};
if (canUseMap) {
itemMap.set(id, item);
return;
}
var key = getKeyFromID(id);
itemByKey[key] = item;
}

function purgeDeep(id) {
Expand Down Expand Up @@ -144,10 +193,6 @@ var ReactComponentTreeHook = {

onBeforeMountComponent(id, element, parentID) {
create(id, element, parentID);

if (parentID === 0) {
rootIDs[id] = true;
}
},

onBeforeUpdateComponent(id, element) {
Expand All @@ -163,6 +208,9 @@ var ReactComponentTreeHook = {
onMountComponent(id) {
var item = get(id);
item.isMounted = true;
if (item.parentID === 0) {
rootIDs.push(id);
}
},

onUpdateComponent(id) {
Expand All @@ -184,9 +232,14 @@ var ReactComponentTreeHook = {
// got a chance to mount, but it still gets an unmounting event during
// the error boundary cleanup.
item.isMounted = false;
if (item.parentID === 0) {
var indexInRootIDs = rootIDs.indexOf(id);
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 do something that's not O(n^2) to unmount n roots? If getRootIDs is only used in tests than the slow implementation you had would be better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, will do on Monday

if (indexInRootIDs !== -1) {
rootIDs.splice(indexInRootIDs, 1);
}
}
}
unmountedIDs[id] = true;
delete rootIDs[id];
unmountedIDs.push(id);
},

purgeUnmountedComponents() {
Expand All @@ -195,10 +248,11 @@ var ReactComponentTreeHook = {
return;
}

for (var id in unmountedIDs) {
for (var i = 0; i < unmountedIDs.length; i++) {
var id = unmountedIDs[i];
purgeDeep(id);
}
unmountedIDs = {};
unmountedIDs.length = 0;
},

isMounted(id) {
Expand Down Expand Up @@ -292,10 +346,13 @@ var ReactComponentTreeHook = {
},

getRootIDs() {
return Object.keys(rootIDs);
return rootIDs;
},

getRegisteredIDs() {
if (canUseMap) {
return Array.from(itemMap.keys());
}
return Object.keys(itemByKey).map(getIDFromKey);
},
};
Expand Down
118 changes: 113 additions & 5 deletions src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,19 @@ describe('ReactComponentTreeHook', () => {
}
}

function expectWrapperTreeToEqual(expectedTree) {
function expectWrapperTreeToEqual(expectedTree, andStayMounted) {
ReactComponentTreeTestUtils.expectTree(rootInstance._debugID, {
displayName: 'Wrapper',
children: expectedTree ? [expectedTree] : [],
});
var rootDisplayNames = ReactComponentTreeTestUtils.getRootDisplayNames();
var registeredDisplayNames = ReactComponentTreeTestUtils.getRegisteredDisplayNames();
if (!expectedTree) {
expect(ReactComponentTreeTestUtils.getRootDisplayNames()).toEqual([]);
expect(ReactComponentTreeTestUtils.getRegisteredDisplayNames()).toEqual([]);
expect(rootDisplayNames).toEqual([]);
expect(registeredDisplayNames).toEqual([]);
} else if (andStayMounted) {
expect(rootDisplayNames).toContain('Wrapper');
expect(registeredDisplayNames).toContain('Wrapper');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added this to verify both getRootDisplayNames() and getRegisteredDisplayNames() work on both code paths.

}
}

Expand All @@ -64,12 +69,12 @@ describe('ReactComponentTreeHook', () => {

// Mount a new tree or update the existing tree.
ReactDOM.render(<Wrapper />, node);
expectWrapperTreeToEqual(expectedTree);
expectWrapperTreeToEqual(expectedTree, true);

// Purging should have no effect
// on the tree we expect to see.
ReactComponentTreeHook.purgeUnmountedComponents();
expectWrapperTreeToEqual(expectedTree);
expectWrapperTreeToEqual(expectedTree, true);
});

// Unmounting the root node should purge
Expand Down Expand Up @@ -1854,4 +1859,107 @@ describe('ReactComponentTreeHook', () => {
ReactDOM.render(<Foo />, el);
});
});

describe('in environment without Map and Array.from', () => {
var realMap;
var realArrayFrom;

beforeEach(() => {
realMap = global.Map;
realArrayFrom = Array.from;

global.Map = undefined;
Array.from = undefined;

jest.resetModuleRegistry();

React = require('React');
ReactDOM = require('ReactDOM');
ReactDOMServer = require('ReactDOMServer');
ReactInstanceMap = require('ReactInstanceMap');
ReactComponentTreeHook = require('ReactComponentTreeHook');
ReactComponentTreeTestUtils = require('ReactComponentTreeTestUtils');
});

afterEach(() => {
global.Map = realMap;
Array.from = realArrayFrom;
});

it('works', () => {
class Qux extends React.Component {
render() {
return null;
}
}

function Foo() {
return {
render() {
return <Qux />;
},
};
}
function Bar({children}) {
return <h1>{children}</h1>;
}
class Baz extends React.Component {
render() {
return (
<div>
<Foo />
<Bar>
<span>Hi,</span>
Mom
</Bar>
<a href="#">Click me.</a>
</div>
);
}
}

var element = <Baz />;
var tree = {
displayName: 'Baz',
element,
children: [{
displayName: 'div',
children: [{
displayName: 'Foo',
element: <Foo />,
children: [{
displayName: 'Qux',
element: <Qux />,
children: [],
}],
}, {
displayName: 'Bar',
children: [{
displayName: 'h1',
children: [{
displayName: 'span',
children: [{
displayName: '#text',
element: 'Hi,',
text: 'Hi,',
}],
}, {
displayName: '#text',
text: 'Mom',
element: 'Mom',
}],
}],
}, {
displayName: 'a',
children: [{
displayName: '#text',
text: 'Click me.',
element: 'Click me.',
}],
}],
}],
};
assertTreeMatches([element, tree]);
});
});
});
15 changes: 10 additions & 5 deletions src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,14 +70,19 @@ describe('ReactComponentTreeHook', () => {
}
}

function expectWrapperTreeToEqual(expectedTree) {
function expectWrapperTreeToEqual(expectedTree, andStayMounted) {
ReactComponentTreeTestUtils.expectTree(rootInstance._debugID, {
displayName: 'Wrapper',
children: expectedTree ? [expectedTree] : [],
});
var rootDisplayNames = ReactComponentTreeTestUtils.getRootDisplayNames();
var registeredDisplayNames = ReactComponentTreeTestUtils.getRegisteredDisplayNames();
if (!expectedTree) {
expect(ReactComponentTreeTestUtils.getRootDisplayNames()).toEqual([]);
expect(ReactComponentTreeTestUtils.getRegisteredDisplayNames()).toEqual([]);
expect(rootDisplayNames).toEqual([]);
expect(registeredDisplayNames).toEqual([]);
} else if (andStayMounted) {
expect(rootDisplayNames).toContain('Wrapper');
expect(registeredDisplayNames).toContain('Wrapper');
}
}

Expand All @@ -88,12 +93,12 @@ describe('ReactComponentTreeHook', () => {

// Mount a new tree or update the existing tree.
ReactNative.render(<Wrapper />, 1);
expectWrapperTreeToEqual(expectedTree);
expectWrapperTreeToEqual(expectedTree, true);

// Purging should have no effect
// on the tree we expect to see.
ReactComponentTreeHook.purgeUnmountedComponents();
expectWrapperTreeToEqual(expectedTree);
expectWrapperTreeToEqual(expectedTree, true);
});

// Unmounting the root node should purge
Expand Down