-
Notifications
You must be signed in to change notification settings - Fork 61
fix(dom): keep readiness gate async-free so TestCafe t.eval accepts the bundle #2346
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| # The @percy/dom bundle is injected into the browser verbatim by SDKs. Some | ||
| # evaluators — notably TestCafe's `t.eval` — statically reject ANY async/await | ||
| # or generator syntax in the evaluated source, so the shipped bundle must stay | ||
| # async-free. Enforce that at authoring time here (this config cascades onto | ||
| # every file in src/); use explicit Promise chains instead (PER-10121). | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Low] Guard silently dies under an ESLint flat-config migration Extensionless Suggestion: add a one-line note in this header comment so a flat-config migration doesn't silently drop the guard (a bundle-AST regression test would also backstop this). Reviewer: stack:code-reviewer |
||
| rules: | ||
| no-restricted-syntax: | ||
| - error | ||
| - selector: ':function[async=true]' | ||
| message: 'async functions are forbidden in @percy/dom src — the bundle is injected via TestCafe t.eval, which rejects async/await. Use explicit Promise chains (PER-10121).' | ||
| - selector: AwaitExpression | ||
| message: 'await is forbidden in @percy/dom src — the bundle is injected via TestCafe t.eval, which rejects async/await. Use explicit Promise chains (PER-10121).' | ||
| - selector: ':function[generator=true]' | ||
| message: 'generator functions are forbidden in @percy/dom src — the bundle is injected via TestCafe t.eval, which rejects generators (PER-10121).' | ||
| - selector: YieldExpression | ||
| message: 'yield is forbidden in @percy/dom src — the bundle is injected via TestCafe t.eval, which rejects generators (PER-10121).' | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Low] Top-level
Suggestion: add a fifth selector: Reviewer: stack:code-reviewer |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -452,7 +452,12 @@ export function createAbortHandle() { | |
| }; | ||
| } | ||
|
|
||
| async function runAllChecks(config, result, aborted) { | ||
| // Returns a Promise rather than using `async`/`await`. The shipped | ||
| // @percy/dom bundle is injected into the browser verbatim by SDKs, and some | ||
| // evaluators — notably TestCafe's `t.eval` — statically reject ANY async/await | ||
| // or generator syntax in the evaluated source. Keeping this file async-free | ||
| // preserves the pre-1.31.14 bundle contract those SDKs depend on (PER-10121). | ||
| function runAllChecks(config, result, aborted) { | ||
| let checks = []; | ||
| let expected = []; | ||
| // dom_stability: false is an explicit kill switch for the MutationObserver | ||
|
|
@@ -476,7 +481,7 @@ async function runAllChecks(config, result, aborted) { | |
| if (config.ready_selectors?.length) { expected.push('ready_selectors'); checks.push(checkReadySelectors(config.ready_selectors, aborted).then(r => { result.checks.ready_selectors = r; })); } | ||
| if (config.not_present_selectors?.length) { expected.push('not_present_selectors'); checks.push(checkNotPresentSelectors(config.not_present_selectors, aborted).then(r => { result.checks.not_present_selectors = r; })); } | ||
| result._expectedChecks = expected; | ||
| await Promise.all(checks); | ||
| return Promise.all(checks); | ||
| } | ||
|
|
||
| // Normalize camelCase config keys (from .percy.yml / SDK options) to the | ||
|
|
@@ -499,9 +504,11 @@ export function normalizeOptions(options = {}) { | |
| }; | ||
| } | ||
|
|
||
| export async function waitForReady(options = {}) { | ||
| // Async-free by design — see the note on `runAllChecks` (PER-10121). Returns | ||
| // a Promise resolving to the readiness result. | ||
| export function waitForReady(options = {}) { | ||
| let presetName = options.preset || 'balanced'; | ||
| if (presetName === 'disabled') return { passed: true, timed_out: false, skipped: true, checks: {} }; | ||
| if (presetName === 'disabled') return Promise.resolve({ passed: true, timed_out: false, skipped: true, checks: {} }); | ||
|
|
||
| let preset = PRESETS[presetName] || PRESETS.balanced; | ||
| // Normalize user options to snake_case, then merge. Only overrides | ||
|
|
@@ -518,39 +525,37 @@ export async function waitForReady(options = {}) { | |
| let settled = false; | ||
| let aborted = createAbortHandle(); | ||
|
|
||
| try { | ||
| await Promise.race([ | ||
| runAllChecks(config, result, aborted).then(() => { settled = true; }), | ||
| new Promise(resolve => setTimeout(() => { | ||
| if (!settled) { | ||
| result.timed_out = true; | ||
| // Abort all running checks — clears intervals, disconnects observers | ||
| aborted.abort(); | ||
| } | ||
| resolve(); | ||
| }, effectiveTimeout)) | ||
| ]); | ||
| } catch (error) { | ||
| return Promise.race([ | ||
| runAllChecks(config, result, aborted).then(() => { settled = true; }), | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Low] Sync throws now escape Previously Suggestion: wrap the call async-free to convert sync throws into rejections: new Promise(resolve => resolve(runAllChecks(config, result, aborted))).then(() => { settled = true; }),Reviewer: stack:code-reviewer |
||
| new Promise(resolve => setTimeout(() => { | ||
| if (!settled) { | ||
| result.timed_out = true; | ||
| // Abort all running checks — clears intervals, disconnects observers | ||
| aborted.abort(); | ||
| } | ||
| resolve(); | ||
| }, effectiveTimeout)) | ||
| ]).catch(error => { | ||
| /* istanbul ignore next: safety net for unexpected errors in readiness checks */ | ||
| result.error = error.message || String(error); | ||
| } | ||
|
|
||
| // Mark any checks that didn't complete before timeout as failed. | ||
| // `_expectedChecks` is always set by runAllChecks, but coverage here | ||
| // depends on whether any expected check was skipped due to timeout. | ||
| /* istanbul ignore next: only falsy when the catch block above fires before runAllChecks sets _expectedChecks */ | ||
| if (result._expectedChecks) { | ||
| for (let name of result._expectedChecks) { | ||
| if (!result.checks[name]) { | ||
| result.checks[name] = { passed: false, timed_out: true }; | ||
| }).then(() => { | ||
| // Mark any checks that didn't complete before timeout as failed. | ||
| // `_expectedChecks` is always set by runAllChecks, but coverage here | ||
| // depends on whether any expected check was skipped due to timeout. | ||
| /* istanbul ignore next: only falsy when the catch block above fires before runAllChecks sets _expectedChecks */ | ||
| if (result._expectedChecks) { | ||
| for (let name of result._expectedChecks) { | ||
| if (!result.checks[name]) { | ||
| result.checks[name] = { passed: false, timed_out: true }; | ||
| } | ||
| } | ||
| delete result._expectedChecks; | ||
| } | ||
| delete result._expectedChecks; | ||
| } | ||
|
|
||
| result.total_duration_ms = Math.round(performance.now() - startTime); | ||
| result.passed = !result.timed_out && !result.error && Object.values(result.checks).every(c => c.passed); | ||
| return result; | ||
| result.total_duration_ms = Math.round(performance.now() - startTime); | ||
| result.passed = !result.timed_out && !result.error && Object.values(result.checks).every(c => c.passed); | ||
| return result; | ||
| }); | ||
| } | ||
|
|
||
| export { PRESETS }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Low] The guard protects source, not the shipped artifact
The real contract is "the built bundle is async-free"; a future bundled dependency, babel helper, or rollup-target change could reintroduce async into
dist/bundle.jswithout anysrc/lint violation.Suggestion: add a small regression test (or CI step) that parses the built bundle with acorn (already in the dependency tree) and asserts zero async/generator nodes — it would have caught the original 1.31.14 regression.
Reviewer: stack:code-reviewer