Skip to content
This repository has been archived by the owner on Oct 1, 2024. It is now read-only.

[react-testing] improved performance by making descendants calculations lazy for element children #1812

Merged
merged 1 commit into from
Apr 7, 2021
Merged

[react-testing] improved performance by making descendants calculations lazy for element children #1812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions packages/react-testing/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
and adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

## [Unreleased]

- Improved the performance of Root wrapper updates [#1812](https://github.com/Shopify/quilt/pull/1812)

## [2.2.2] - 2021-03-03

### Fixed
Expand Down
48 changes: 36 additions & 12 deletions packages/react-testing/src/element.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,24 @@ export class Element<Props> implements Node<Props> {
return this.elementChildren;
}

private get elementDescendants() {
if (!this.descendantsCache) {
this.descendantsCache = getDescendants(this);
}

return this.descendantsCache;
}

private get elementChildren() {
if (!this.elementChildrenCache) {
this.elementChildrenCache = this.allChildren.filter(
element => typeof element !== 'string',
) as Element<unknown>[];
}

return this.elementChildrenCache;
}

get descendants() {
return this.elementDescendants;
}
Expand All @@ -71,23 +89,14 @@ export class Element<Props> implements Node<Props> {
return domNodes[0] || null;
}

private readonly elementChildren: Element<unknown>[];
private readonly elementDescendants: Element<unknown>[];
private descendantsCache: Element<unknown>[] | null = null;
private elementChildrenCache: Element<unknown>[] | null = null;

constructor(
private readonly tree: Tree<Props>,
private readonly allChildren: (Element<unknown> | string)[],
allDescendants: (Element<unknown> | string)[],
public readonly root: Root,
) {
this.elementChildren = allChildren.filter(
element => typeof element !== 'string',
) as Element<unknown>[];

this.elementDescendants = allDescendants.filter(
element => typeof element !== 'string',
) as Element<unknown>[];
}
) {}

data(key: string): string | undefined {
return this.props[key.startsWith('data-') ? key : `data-${key}`];
Expand Down Expand Up @@ -252,3 +261,18 @@ function equalSubset(subset: object, full: object) {
key => key in full && full[key] === subset[key],
);
}

function getDescendants(element: any) {
const descendants: Element<unknown>[] = [];
// eslint-disable-next-line @typescript-eslint/prefer-for-of
atesgoral marked this conversation as resolved.
Show resolved Hide resolved
for (let i = 0; i < element.allChildren.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could change our compilation settings for this package to have the foreach version of this not get transpiled, this is a node library, and node definitely supports foreach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried targeting ES2015 and changing that function to a more simplified:

function getDescendants(element: any) {
  const descendants: Element<unknown>[] = [];
  for (const child of element.allChildren) {
    if (typeof child !== 'string') {
      descendants.push(child, ...child.elementDescendants);
    }
  }

  return descendants;
}

but it is unfortunately considerably slower :(

for of with spread operator:

Screen Shot 2021-04-05 at 11 42 26 AM

for loop with no spread:

Screen Shot 2021-04-05 at 11 42 05 AM

(There's a slight variance between runs. I run these tests several times, wait for them to stabilize and take the lowest run time)

const child = element.allChildren[i];
if (typeof child !== 'string') {
descendants.push(child);
// eslint-disable-next-line prefer-spread
descendants.push.apply(descendants, child.elementDescendants);
}
}

return descendants;
}
64 changes: 27 additions & 37 deletions packages/react-testing/src/root.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,10 +253,12 @@ export class Root<Props> implements Node<Props> {
if (this.wrapper == null) {
this.root = null;
} else {
const topElement = flatten(
((this.wrapper as unknown) as ReactInstance)._reactInternalFiber,
const topElement = fiberToElement(
findCurrentFiberUsingSlowPath(
((this.wrapper as unknown) as ReactInstance)._reactInternalFiber,
),
this,
)[0];
);

this.root = this.resolveRoot(topElement as any) as any;
}
Expand Down Expand Up @@ -284,52 +286,40 @@ function defaultRender(element: React.ReactElement<unknown>) {
return element;
}

function flatten(
element: Fiber,
function fiberToElement(
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for rename, the preact version of the library I did similar renames :)

node: Fiber,
root: Root<unknown>,
): (Element<unknown> | string)[] {
const node: Fiber = findCurrentFiberUsingSlowPath(element);

): Element<unknown> | string {
if (node.tag === Tag.HostText) {
return [node.memoizedProps as string];
}

const props = {...((node.memoizedProps as any) || {})};
const {children, descendants} = childrenToTree(node.child, root);

return [
new Element(
{
tag: node.tag,
type: node.type,
props,
instance: node.stateNode,
},
children,
descendants,
root,
),
...descendants,
];
return node.memoizedProps as string;
}

const props = node.memoizedProps as any;
const children = childrenToTree(node.child, root);

return new Element(
{
tag: node.tag,
type: node.type,
props,
instance: node.stateNode,
},
children,
root,
);
}

function childrenToTree(fiber: Fiber | null, root: Root<unknown>) {
let currentFiber = fiber;
const children: (string | Element<unknown>)[] = [];
const descendants: (string | Element<unknown>)[] = [];

while (currentFiber != null) {
const result = flatten(currentFiber, root);

if (result.length > 0) {
children.push(result[0]);
descendants.push(...result);
}

const result = fiberToElement(currentFiber, root);
children.push(result);
currentFiber = currentFiber.sibling;
}

return {children, descendants};
return children;
}

function isPromise<T>(promise: T | Promise<T>): promise is Promise<T> {
Expand Down
Loading