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:
Neither patch is gated, so both nest a layer deeper per spec file —
`setupFiles` re-runs and the window survives `isolate: false`. Tagged each
wrapper: 41 layers deep by the end of a full run on 8 cores, deeper on CI where
workers hold more files. Same accumulation the file already guards at lines 33
and 292-295; a third `Symbol.for` flag around 207-232 covers 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 `restoreOwnerScroll`
(html2canvas.js:5652), guarded by `x !== pageXOffset` — both 0 under jsdom.
Deleted these two lines and ran all 201 files: zero `Not implemented:
window.scrollTo`. The comment's "one `scrollTo` per document clone" is the
iframe's, handled just below.
##########
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` (html2canvas.js:5212), so `<head>`
gets walked too and isn't parked. Measured at this hook over a full run: body
peaks at 463 descendants, head at 185, with 46 of 201 runs above 76. Parking
`document.head.childNodes` as well would close that ~29% residual.
##########
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` runs only on the success path (html2canvas.js:7794,
no try/finally), so each failed render leaks its clone iframe. Measured three
`iframe.html2canvas-container` still in body after this hook — live browsing
contexts every later spec file inherits. Pre-existing, but this is the hook for
it:
```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 `generateWorkflowSnapshot`, not the file — and a render
that outlives `afterAll` sees the restored DOM, so "has to span the file"
doesn't hold at the suite boundary either. 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:
Not quite no behaviour change: jsdom throws `TypeError` for shadow-DOM
pseudo-elements (Window.js:890-892) *before* `notImplemented`, and dropping the
argument loses that. Latent — nothing in `src` passes a pseudo-element.
--
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]
