Repository: preactjs/preact-devtools
Pull request #167 (2020-04-21 20:40:12)

Fix hooks parsing error when value is shortened
This was caused by a bit of technical dept in our props parser. It wrongly determined node depth by looking at the amount of dots in the id. This PR removes that weird assumption and instead bases the depth on the actual tree hierarchy.


Patch:
diff --git a/src/adapter/10/renderer/hooks.ts b/src/adapter/10/renderer/hooks.ts
index b2b7d26..ba27c72 100644
--- a/src/adapter/10/renderer/hooks.ts
+++ b/src/adapter/10/renderer/hooks.ts
@@ -136,9 +136,10 @@ export function parseHookData(
 			if (!tree.has(id)) {
 				let value: any = "__preact_empty__";
 				let editable = false;
-				let children = [];
+				let children: string[] = [];
 				let nodeType: PropDataType = "undefined";
 				const depth = hook.stack.length - i - 1;
+				const name = isNative ? type : frame.name;
 
 				if (debugValues.has(id)) {
 					value = debugValues.get(id);
@@ -155,18 +156,18 @@ export function parseHookData(
 					// properties if the value is an object. We parse it
 					// separately and append it as children to our hook node
 					if (typeof rawValue === "object") {
-						const tree = new Map();
-						parseProps(value, id, 7, tree);
+						const tree = parseProps(value, id, 7, 0, name);
 						children = tree.get(id)!.children;
-						hookValueTree = Array.from(tree.values()).slice(1);
+						hookValueTree = Array.from(tree.values());
+						if (hookValueTree.length > 1) {
+							hookValueTree = hookValueTree.slice(1);
+						}
 						nodeType = hookValueTree[0].type;
 
-						// TODO: Parse props relies on dots in id
-						const wrongDepth = (id.match(/\./g) || []).length;
-
 						hookValueTree.forEach(node => {
+							node.id = id + node.id;
 							node.editable = false;
-							node.depth = node.depth - wrongDepth + depth;
+							node.depth += depth;
 						});
 					}
 					editable =
@@ -180,7 +181,7 @@ export function parseHookData(
 					depth,
 					editable,
 					id,
-					name: isNative ? type : frame.name,
+					name,
 					type: nodeType,
 					meta: isNative
 						? {
diff --git a/src/view/components/sidebar/inspect/parseProps.ts b/src/view/components/sidebar/inspect/parseProps.ts
index cc6f600..7ac209b 100644
--- a/src/view/components/sidebar/inspect/parseProps.ts
+++ b/src/view/components/sidebar/inspect/parseProps.ts
@@ -29,11 +29,10 @@ export function parseProps(
 	data: any,
 	path: string,
 	limit: number,
-	out: Map<string, PropData>,
+	depth = 0,
+	name = path,
+	out = new Map<string, PropData>(),
 ): Map<string, PropData> {
-	const depth = (path.match(/\./g) || []).length;
-	const name = path.slice(path.lastIndexOf(".") + 1);
-
 	if (depth >= limit) {
 		out.set(path, {
 			depth,
@@ -63,7 +62,7 @@ export function parseProps(
 		data.forEach((item, i) => {
 			const childPath = `${path}.${i}`;
 			children.push(childPath);
-			parseProps(item, childPath, limit, out);
+			parseProps(item, childPath, limit, depth + 1, "" + i, out);
 		});
 	} else if (data instanceof Set) {
 		// TODO: We're dealing with serialized data here, not a Set object
@@ -187,7 +186,7 @@ export function parseProps(
 				Object.keys(data).forEach(key => {
 					const nextPath = `${path}.${key}`;
 					node.children.push(nextPath);
-					parseProps(data[key], nextPath, limit, out);
+					parseProps(data[key], nextPath, limit, depth + 1, key, out);
 				});
 
 				out.set(path, node);
diff --git a/src/view/store/props.ts b/src/view/store/props.ts
index dbc43dd..d1d2b9d 100644
--- a/src/view/store/props.ts
+++ b/src/view/store/props.ts
@@ -29,7 +29,7 @@ export function parseObjectState(
 			meta: null,
 		});
 
-		parseProps(data, "root", PROPS_LIMIT, tree);
+		parseProps(data, "root", PROPS_LIMIT, 0, "root", tree);
 		const { items } = flattenChildren(tree, "root", id => {
 			return tree.get(id)!.children.length > 0 && isCollapsed(uncollapsed, id);
 		});


Test patch:
diff --git a/src/view/components/sidebar/inspect/parseProps.test.ts b/src/view/components/sidebar/inspect/parseProps.test.ts
index 670af94..d031a5c 100644
--- a/src/view/components/sidebar/inspect/parseProps.test.ts
+++ b/src/view/components/sidebar/inspect/parseProps.test.ts
@@ -5,8 +5,7 @@ const serialize = (v: Map<any, any>) => Array.from(v.values());
 
 describe("parseProps", () => {
 	it("should flatten strings", () => {
-		const tree = new Map();
-		parseProps("foo", "foo", 2, tree);
+		const tree = parseProps("foo", "foo", 2);
 
 		expect(serialize(tree)).to.deep.equal([
 			{
@@ -23,8 +22,7 @@ describe("parseProps", () => {
 	});
 
 	it("should flatten numbers", () => {
-		const tree = new Map();
-		parseProps(12, "foo", 2, tree);
+		const tree = parseProps(12, "foo", 2);
 
 		expect(serialize(tree)).to.deep.equal([
 			{
@@ -41,8 +39,7 @@ describe("parseProps", () => {
 	});
 
 	it("should flatten booleans", () => {
-		const tree = new Map();
-		parseProps(false, "foo", 2, tree);
+		const tree = parseProps(false, "foo", 2);
 
 		expect(serialize(tree)).to.deep.equal([
 			{
@@ -59,8 +56,7 @@ describe("parseProps", () => {
 	});
 
 	it("should flatten null", () => {
-		const tree = new Map();
-		parseProps(null, "foo", 2, tree);
+		const tree = parseProps(null, "foo", 2);
 		expect(serialize(tree)).to.deep.equal([
 			{
 				editable: false,
@@ -76,8 +72,7 @@ describe("parseProps", () => {
 	});
 
 	it("should flatten undefined", () => {
-		const tree = new Map();
-		parseProps(undefined, "foo", 2, tree);
+		const tree = parseProps(undefined, "foo", 2);
 		expect(serialize(tree)).to.deep.equal([
 			{
 				editable: false,
@@ -93,12 +88,11 @@ describe("parseProps", () => {
 	});
 
 	it("should parse functions", () => {
-		const tree = new Map();
 		const fn = {
 			type: "function",
 			name: "fooBar",
 		};
-		parseProps(fn, "foo", 2, tree);
+		const tree = parseProps(fn, "foo", 2);
 		expect(serialize(tree)).to.deep.equal([
 			{
 				editable: false,
@@ -114,8 +108,7 @@ describe("parseProps", () => {
 	});
 
 	it("should flatten arrays", () => {
-		const tree = new Map();
-		parseProps([1, 2], "foo", 2, tree);
+		const tree = parseProps([1, 2], "foo", 2);
 		expect(serialize(tree)).to.deep.equal([
 			{
 				editable: false,
@@ -151,8 +144,7 @@ describe("parseProps", () => {
 	});
 
 	it("should not mark empty arrays as collabsible", () => {
-		const tree = new Map();
-		parseProps([], "foo", 2, tree);
+		const tree = parseProps([], "foo", 2);
 		expect(serialize(tree)).to.deep.equal([
 			{
 				editable: false,
@@ -168,8 +160,7 @@ describe("parseProps", () => {
 	});
 
 	it("should flatten objects", () => {
-		const tree = new Map();
-		parseProps({ foo: 123, bar: "abc" }, "", 2, tree);
+		const tree = parseProps({ foo: 123, bar: "abc" }, "", 2);
 		expect(serialize(tree)).to.deep.equal([
 			{
 				editable: false,
@@ -208,8 +199,7 @@ describe("parseProps", () => {
 	});
 
 	it("should flatten nested objects", () => {
-		const tree = new Map();
-		parseProps({ foo: { bar: "abc" } }, "foo", 4, tree);
+		const tree = parseProps({ foo: { bar: "abc" } }, "foo", 4);
 		expect(serialize(tree)).to.deep.equal([
 			{
 				editable: false,
@@ -251,8 +241,7 @@ describe("parseProps", () => {
 	});
 
 	it("should limit depth", () => {
-		const tree = new Map();
-		parseProps({ foo: { bar: { boof: "abc" } } }, "foo", 2, tree);
+		const tree = parseProps({ foo: { bar: { boof: "abc" } } }, "foo", 2);
 		expect(serialize(tree)).to.deep.equal([
 			{
 				editable: false,
@@ -294,8 +283,7 @@ describe("parseProps", () => {
 	});
 
 	it("should not mark [[Circular]] reference as editable", () => {
-		const tree = new Map();
-		parseProps("[[Circular]]", "", 2, tree);
+		const tree = parseProps("[[Circular]]", "", 2);
 		expect(serialize(tree)).to.deep.equal([
 			{
 				editable: false,
diff --git a/test-e2e/test-utils.ts b/test-e2e/test-utils.ts
index 0d0de31..b9266c0 100644
--- a/test-e2e/test-utils.ts
+++ b/test-e2e/test-utils.ts
@@ -1,4 +1,5 @@
 import { newPage } from "pentf/browser_utils";
+import { wait } from "pentf/utils";
 import fs from "fs";
 import path from "path";
 import { Request, Page } from "puppeteer";
@@ -280,3 +281,100 @@ export async function installMouseHelper(page: Page) {
 		document.addEventListener("mouseup", handleEvent, true);
 	});
 }
+
+// TODO: Remove this once the next version of pentf is released
+export async function clickNestedText(
+	page: Page,
+	textOrRegExp: string | RegExp,
+	{
+		timeout = 30000,
+		checkEvery = 200,
+		extraMessage = undefined,
+		visible = true,
+	} = {},
+) {
+	const serializedMatcher =
+		typeof textOrRegExp !== "string"
+			? { source: textOrRegExp.source, flags: textOrRegExp.flags }
+			: textOrRegExp;
+
+	let remainingTimeout = timeout;
+	// eslint-disable-next-line no-constant-condition
+	while (true) {
+		const found = await page.evaluate(
+			(matcher, visible) => {
+				// eslint-disable-next-line no-undef
+				let matchFunc: (text: string) => boolean;
+				let matchFuncExact: null | ((text: string) => boolean) = null;
+
+				if (typeof matcher == "string") {
+					matchFunc = (text: string) => text.includes(matcher);
+				} else {
+					const regexExact = new RegExp(matcher.source, matcher.flags);
+					matchFuncExact = (text: string) => {
+						// Reset regex state in case global flag was used
+						regexExact.lastIndex = 0;
+						return regexExact.test(text);
+					};
+
+					// Remove leading ^ and ending $, otherwise the traversal
+					// will fail at the first node.
+					const source = matcher.source.replace(/^[^]/, "").replace(/[$]$/, "");
+					const regex = new RegExp(source, matcher.flags);
+					matchFunc = (text: string) => {
+						// Reset regex state in case global flag was used
+						regex.lastIndex = 0;
+						return regex.test(text);
+					};
+				}
+
+				const stack = [document.body];
+				let item = null;
+				let lastFound = null;
+				while ((item = stack.pop())) {
+					// eslint-disable-line no-cond-assign
+					for (let i = 0; i < item.childNodes.length; i++) {
+						const child = item.childNodes[i];
+
+						// Skip text nodes as they are not clickable
+						if (child.nodeType === Node.TEXT_NODE) {
+							continue;
+						}
+
+						const text = child.textContent || "";
+						if (child.childNodes.length > 0 && matchFunc(text)) {
+							if (matchFuncExact === null || matchFuncExact(text)) {
+								lastFound = child as any;
+							}
+							stack.push(child as any);
+						}
+					}
+				}
+
+				if (!lastFound) return false;
+
+				if (visible && lastFound!.offsetParent === null) return null; // invisible)
+
+				lastFound!.click();
+				return true;
+			},
+			serializedMatcher,
+			visible,
+		);
+
+		if (found) {
+			return;
+		}
+
+		if (remainingTimeout <= 0) {
+			const extraMessageRepr = extraMessage ? ` (${extraMessage})` : "";
+			throw new Error(
+				`Unable to find${
+					visible ? " visible" : ""
+				} text "${textOrRegExp}" after ${timeout}ms${extraMessageRepr}`,
+			);
+		}
+		await wait(Math.min(remainingTimeout, checkEvery));
+		remainingTimeout -= checkEvery;
+	}
+}
diff --git a/test-e2e/tests/fixtures/hooks-depth-limit.js b/test-e2e/tests/fixtures/hooks-depth-limit.js
new file mode 100644
index 0000000..ad1c097
--- /dev/null
+++ b/test-e2e/tests/fixtures/hooks-depth-limit.js
@@ -0,0 +1,40 @@
+const { render } = preact;
+const { useState } = preactHooks;
+
+const useFoo = () =>
+	useState({
+		key1: {
+			key2: {
+				key3: {
+					key4: {
+						key5: {
+							key6: {
+								key7: {
+									key8: {
+										key9: 123,
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	});
+const useBar = () => useFoo();
+const useBob = () => useBar();
+const useBoof = () => useBob();
+const useBlub = () => useBoof();
+const useBread = () => useBlub();
+const useBubby = () => useBread();
+const useBabby = () => useBubby();
+const useBleb = () => useBabby();
+const useBlaBla = () => useBleb();
+const useBrobba = () => useBlaBla();
+
+function Hook() {
+	const [v] = useBrobba();
+	return html`<p>${JSON.stringify(v)}</p>`;
+}
+
+render(html`<${Hook} />`, document.getElementById("app"));
diff --git a/test-e2e/tests/hooks/hooks-depth-limit.test.ts b/test-e2e/tests/hooks/hooks-depth-limit.test.ts
new file mode 100644
index 0000000..95c9597
--- /dev/null
+++ b/test-e2e/tests/hooks/hooks-depth-limit.test.ts
@@ -0,0 +1,54 @@
+import { newTestPage, clickNestedText } from "../../test-utils";
+import { closePage, waitForTestId, getText } from "pentf/browser_utils";
+import { expect } from "chai";
+
+export const description =
+	"Show a deeply nested hook tree and limit value parsing depth";
+
+export async function run(config: any) {
+	const { page, devtools } = await newTestPage(config, "hooks-depth-limit", {
+		preact: "hook",
+	});
+
+	await waitForTestId(devtools, "tree-item");
+
+	// State update
+	await clickNestedText(devtools, "Hook", {
+		timeout: 4000,
+	});
+	await devtools.waitForSelector('[data-testid="props-row"]', {
+		timeout: 2000,
+	});
+
+	const options = { timeout: 2000 };
+
+	await clickNestedText(devtools, "useBrobba", options);
+	await clickNestedText(devtools, "useBlaBla", options);
+	await clickNestedText(devtools, "useBleb", options);
+	await clickNestedText(devtools, "useBabby", options);
+	await clickNestedText(devtools, "useBubby", options);
+	await clickNestedText(devtools, "useBread", options);
+	await clickNestedText(devtools, "useBlub", options);
+	await clickNestedText(devtools, "useBoof", options);
+	await clickNestedText(devtools, "useBob", options);
+	await clickNestedText(devtools, "useBar", options);
+	await clickNestedText(devtools, "useFoo", options);
+
+	// Native hooks
+	await clickNestedText(devtools, "useState", options);
+	await clickNestedText(devtools, /^key1/, options);
+	await clickNestedText(devtools, /^key2/, options);
+	await clickNestedText(devtools, /^key3/, options);
+	await clickNestedText(devtools, /^key4/, options);
+	await clickNestedText(devtools, /^key5/, options);
+	await clickNestedText(devtools, /^key6/, options);
+	await clickNestedText(devtools, /^key7/, options);
+
+	const text = await getText(
+		devtools,
+		'form [data-testid="props-row"]:last-child [data-testid="prop-value"]',
+	);
+	expect(text).to.equal('"…"');
+
+	await closePage(page);
+}