aglinxinyuan commented on code in PR #7999:
URL: https://github.com/apache/texera/pull/7999#discussion_r3866919021
##########
frontend/src/jsdom-svg-polyfill.ts:
##########
@@ -192,6 +192,45 @@ G.ResizeObserver ??= class {
disconnect(): void {}
};
+// `window.getComputedStyle(elt, pseudoElt)` and `window.scrollTo` — jsdom
+// implements neither. Both route through its `notImplemented` helper, which
+// emits a `jsdomError` on the virtual console; vitest's jsdom environment
+// forwards that to `console.error`, one full stack trace per call. html2canvas
+// calls both on every render — a `:before` and an `:after` lookup for each
+// cloned node, plus one `scrollTo` per document clone — so the
+// report-generation spec, which drives the real renderer on purpose (`vi.mock`
+// can't reach it under the builder's `isolate: false`), buries the run in
+// traces while all of its tests pass.
+// Dropping `pseudoElt` changes no behaviour: jsdom complains and then ignores
+// the argument, returning the element's own declaration either way. `scrollTo`
+// has nothing to move — jsdom has no layout.
+const jsdomGetComputedStyle = G.getComputedStyle as ((elt: Element,
pseudoElt?: string | null) => unknown) | undefined;
Review Comment:
Not gated, so both patches nest a layer deeper per spec file — 41 deep by
the end of a full run. Same accumulation the file already guards at lines 33
and 292; a `Symbol.for` flag around 207-232 fixes both.
##########
frontend/src/jsdom-svg-polyfill.ts:
##########
@@ -192,6 +192,45 @@ G.ResizeObserver ??= class {
disconnect(): void {}
};
+// `window.getComputedStyle(elt, pseudoElt)` and `window.scrollTo` — jsdom
+// implements neither. Both route through its `notImplemented` helper, which
+// emits a `jsdomError` on the virtual console; vitest's jsdom environment
+// forwards that to `console.error`, one full stack trace per call. html2canvas
+// calls both on every render — a `:before` and an `:after` lookup for each
+// cloned node, plus one `scrollTo` per document clone — so the
+// report-generation spec, which drives the real renderer on purpose (`vi.mock`
+// can't reach it under the builder's `isolate: false`), buries the run in
+// traces while all of its tests pass.
+// Dropping `pseudoElt` changes no behaviour: jsdom complains and then ignores
+// the argument, returning the element's own declaration either way. `scrollTo`
+// has nothing to move — jsdom has no layout.
+const jsdomGetComputedStyle = G.getComputedStyle as ((elt: Element,
pseudoElt?: string | null) => unknown) | undefined;
+if (typeof jsdomGetComputedStyle === "function") {
+ const withoutPseudoElement = ((elt: Element) => jsdomGetComputedStyle(elt))
as AnyFn;
+ G.getComputedStyle = withoutPseudoElement;
+ if (G.window) G.window.getComputedStyle = withoutPseudoElement;
+}
+const inertScrollTo: AnyFn = () => undefined;
+G.scrollTo = inertScrollTo;
Review Comment:
Dead code — html2canvas's only main-window `scrollTo` is guarded by `x !==
pageXOffset`, both 0 under jsdom. Dropped these two lines, ran all 201 files,
zero traces.
##########
frontend/src/app/workspace/service/report-generation/report-generation.service.spec.ts:
##########
@@ -330,6 +330,28 @@ describe("ReportGenerationService", () => {
});
describe("generateWorkflowSnapshot", () => {
+ /**
+ * html2canvas clones the whole document, not just the element it is
pointed at, and the
+ * unit-test builder runs spec files with `isolate: false` — so one jsdom
document is shared
+ * by every spec file in a worker, and the renders below drag in whatever
DOM the files
+ * before this one left behind. That is what failed the macOS leg: a clone
that costs ~60ms
+ * against this file's own DOM was measured at 12–37s there, past the 20s
test timeout,
+ * while ubuntu and windows passed. Park the foreign nodes for the
duration of the file and
+ * put them back after, so the render's cost depends only on what these
tests build. The
+ * renders started here outlive the tests that start them, so this has to
span the file
+ * rather than each test.
+ */
+ let parkedNodes: ChildNode[];
+
+ beforeAll(() => {
+ parkedNodes = Array.from(document.body.childNodes);
Review Comment:
html2canvas clones from `documentElement`, so unparked `<head>` is walked
too — measured 185 elements there against body's 463. Park
`document.head.childNodes` as well?
##########
frontend/src/app/workspace/service/report-generation/report-generation.service.spec.ts:
##########
@@ -330,6 +330,28 @@ describe("ReportGenerationService", () => {
});
describe("generateWorkflowSnapshot", () => {
+ /**
+ * html2canvas clones the whole document, not just the element it is
pointed at, and the
+ * unit-test builder runs spec files with `isolate: false` — so one jsdom
document is shared
+ * by every spec file in a worker, and the renders below drag in whatever
DOM the files
+ * before this one left behind. That is what failed the macOS leg: a clone
that costs ~60ms
+ * against this file's own DOM was measured at 12–37s there, past the 20s
test timeout,
+ * while ubuntu and windows passed. Park the foreign nodes for the
duration of the file and
+ * put them back after, so the render's cost depends only on what these
tests build. The
+ * renders started here outlive the tests that start them, so this has to
span the file
+ * rather than each test.
+ */
+ let parkedNodes: ChildNode[];
+
+ beforeAll(() => {
+ parkedNodes = Array.from(document.body.childNodes);
+ parkedNodes.forEach(node => node.remove());
+ });
+
+ afterAll(() => {
+ parkedNodes.forEach(node => document.body.appendChild(node));
Review Comment:
`DocumentCloner.destroy` only runs on the success path, so each failed
render leaks its clone iframe — three left in body after this hook. Worth
removing them here too:
```ts
document.body.querySelectorAll("iframe.html2canvas-container").forEach(node
=> node.remove());
```
##########
frontend/src/app/workspace/service/report-generation/report-generation.service.spec.ts:
##########
@@ -330,6 +330,28 @@ describe("ReportGenerationService", () => {
});
describe("generateWorkflowSnapshot", () => {
+ /**
+ * html2canvas clones the whole document, not just the element it is
pointed at, and the
+ * unit-test builder runs spec files with `isolate: false` — so one jsdom
document is shared
+ * by every spec file in a worker, and the renders below drag in whatever
DOM the files
+ * before this one left behind. That is what failed the macOS leg: a clone
that costs ~60ms
+ * against this file's own DOM was measured at 12–37s there, past the 20s
test timeout,
+ * while ubuntu and windows passed. Park the foreign nodes for the
duration of the file and
+ * put them back after, so the render's cost depends only on what these
tests build. The
+ * renders started here outlive the tests that start them, so this has to
span the file
Review Comment:
These hooks scope to the suite, not the file — and a render outliving
`afterAll` sees the restored DOM either way. Suggest "this suite".
##########
frontend/src/jsdom-svg-polyfill.ts:
##########
@@ -192,6 +192,45 @@ G.ResizeObserver ??= class {
disconnect(): void {}
};
+// `window.getComputedStyle(elt, pseudoElt)` and `window.scrollTo` — jsdom
+// implements neither. Both route through its `notImplemented` helper, which
+// emits a `jsdomError` on the virtual console; vitest's jsdom environment
+// forwards that to `console.error`, one full stack trace per call. html2canvas
+// calls both on every render — a `:before` and an `:after` lookup for each
+// cloned node, plus one `scrollTo` per document clone — so the
+// report-generation spec, which drives the real renderer on purpose (`vi.mock`
+// can't reach it under the builder's `isolate: false`), buries the run in
+// traces while all of its tests pass.
+// Dropping `pseudoElt` changes no behaviour: jsdom complains and then ignores
+// the argument, returning the element's own declaration either way. `scrollTo`
+// has nothing to move — jsdom has no layout.
+const jsdomGetComputedStyle = G.getComputedStyle as ((elt: Element,
pseudoElt?: string | null) => unknown) | undefined;
+if (typeof jsdomGetComputedStyle === "function") {
+ const withoutPseudoElement = ((elt: Element) => jsdomGetComputedStyle(elt))
as AnyFn;
Review Comment:
jsdom throws `TypeError` for shadow-DOM pseudo-elements *before*
`notImplemented`, so dropping the argument does change behaviour. Latent —
nothing in `src` passes one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
