feat(browser): scope waitFor rules by page type instead of a duplicated path regex (v1.17.0) - #71
feat(browser): scope waitFor rules by page type instead of a duplicated path regex (v1.17.0)#71harper-joseph wants to merge 1 commit into
Conversation
…ed path regex; v1.17.0
`waitFor[].pathPattern` describes which pages have a widget as a regex over the
URL path — a SECOND description of a routing fact the plugin's route list already
owns, maintained in a different repository. The two drift silently: add a route to
a template on the plugin side and it renders without the rule, with nothing
reporting the gap.
The plugin (prerender-v0.34.0) now declares named page types and sends the matched
name on each queue job. This consumes it:
RenderJob carries `pageType` from the job payload (absent on older plugins).
WaitForRule gains `pageTypes?: string[]`, ANDed with `devices`. A job with no
declared type matches no such rule — skipping costs that rule's
content, while applying it blindly would restore the poll-to-
timeout on every page lacking the widget that scoping exists to
prevent.
renderOnce gains a `pageType` option, so the harness reproduces a scoped rule
instead of silently rendering what the fleet does not.
renderAudit its `pageType` was a REPORT LABEL that never reached the render.
Left alone, a pageTypes-scoped rule would be skipped on every
audit render, so state B would settle differently from the fleet
it reproduces and the audit would report content as missing that
production captures. It now scopes the render too.
suggest emits `pageTypes: [name]` over `pathPattern` when the audit was
given a type — a suggested regex is the same second copy of the
route list, drifting from the moment a route is added.
`pathPattern` still works and is not deprecated; migrate rule by rule.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces pageTypes scoping for waitFor rules in @harperfast/prerender-browser, allowing rules to be scoped by template names (such as pdp or category) instead of relying solely on regex-based pathPatterns. This change includes updates to configuration validation, the renderer, audit tools, and documentation. The review feedback recommends enhancing configuration validation to reject empty pageTypes arrays, adding corresponding test coverage, and simplifying a logical expression in the renderer to improve readability.
| if ( | ||
| rule.pageTypes !== undefined && | ||
| (!Array.isArray(rule.pageTypes) || rule.pageTypes.some((t) => typeof t !== 'string' || t.trim() === '')) | ||
| ) { | ||
| throw new Error(`prerender config: waitFor[${i}].pageTypes must be an array of non-empty type names`); | ||
| } |
There was a problem hiding this comment.
The validation check for rule.pageTypes does not prevent an empty array ([]) from being configured. If pageTypes is configured as an empty array, the rule will always be skipped silently because rule.pageTypes.includes(job.pageType) will always evaluate to false. To prevent this silent misconfiguration, we should explicitly check that rule.pageTypes.length === 0 is also treated as an invalid configuration.
| if ( | |
| rule.pageTypes !== undefined && | |
| (!Array.isArray(rule.pageTypes) || rule.pageTypes.some((t) => typeof t !== 'string' || t.trim() === '')) | |
| ) { | |
| throw new Error(`prerender config: waitFor[${i}].pageTypes must be an array of non-empty type names`); | |
| } | |
| if ( | |
| rule.pageTypes !== undefined && | |
| (!Array.isArray(rule.pageTypes) || rule.pageTypes.length === 0 || rule.pageTypes.some((t) => typeof t !== 'string' || t.trim() === '')) | |
| ) { | |
| throw new Error("prerender config: waitFor[" + i + "].pageTypes must be an array of non-empty type names"); | |
| } |
| // predate the field, or the route may name no template. Skipping costs the content this | ||
| // rule would have waited for; applying it blindly would cost a poll to the timeout on | ||
| // every page that lacks the widget, which is the failure this scoping exists to prevent. | ||
| if (rule.pageTypes && !(job.pageType && rule.pageTypes.includes(job.pageType))) continue; |
There was a problem hiding this comment.
The logical expression !(job.pageType && rule.pageTypes.includes(job.pageType)) is a bit hard to read and can evaluate to a non-boolean value (e.g., undefined or null) before the logical NOT operator ! coerces it. It is cleaner and more idiomatic to explicitly check both conditions under which the rule should be skipped: either the job has no page type, or the job's page type is not included in the allowed list.
| if (rule.pageTypes && !(job.pageType && rule.pageTypes.includes(job.pageType))) continue; | |
| if (rule.pageTypes && (!job.pageType || !rule.pageTypes.includes(job.pageType))) continue; |
| assert.throws( | ||
| () => mergeConfig({ waitFor: [{ selector: '#r', pageTypes: ['pdp', ''] }] }), | ||
| /waitFor\[0\]\.pageTypes must be an array/ | ||
| ); |
There was a problem hiding this comment.
Let's add a test case to verify that configuring pageTypes as an empty array ([]) is correctly rejected by the validation logic.
assert.throws(
() => mergeConfig({ waitFor: [{ selector: '#r', pageTypes: ['pdp', ''] }] }),
/waitFor\\[0\\]\\.pageTypes must be an array/
);
assert.throws(
() => mergeConfig({ waitFor: [{ selector: '#r', pageTypes: [] }] }),
/waitFor\\[0\\]\\.pageTypes must be an array/
);1 participant
Consumes the page types introduced in #70. Independent of that PR — nothing here requires it to merge first; a job without
pageTypesimply carries none.The problem
waitFor[].pathPatternsays which pages have a widget as a regex over the URL path:That is a second description of a routing fact the plugin's route list already owns, maintained in a different repository. The two drift silently — add a route to that template on the plugin side and it renders without the rule, with nothing reporting the gap. It also can't express cheaply what a template reached by several unrelated URL shapes needs: an alternation kept in step by hand.
The change
The plugin now sends the matched template name on each queue job. This consumes it:
RenderJobpageTypefrom the job payload (absent on plugins <prerender-v0.34.0)config.tsWaitForRule.pageTypes?: string[], validated; ANDs withdevicesrenderer.tsrenderOncepageTypeoption, so the harness reproduces a scoped ruleaudit/renderAuditpageTypenow reaches the render, not just the reportaudit/suggestpageTypes: [name]overpathPatternwhen given a typeTwo decisions worth review
A job with no declared page type matches no
pageTypesrule. Skipping costs the content that one rule would have waited for. Applying it blindly would restore the poll-to-timeout on every page lacking the widget — the exact failure scoping exists to prevent, and measured at ~+15s per non-PDP render. Skipping is the safe direction.renderAudit.pageTypewas a report label that never reached the render. Harmless while nothing in a render consulted it. Left alone here, apageTypes-scoped rule would be skipped on every audit render, so state B would settle differently from the fleet it is supposed to reproduce — and the audit would report content as missing that production actually captures. It now scopes the render too (empty string →undefined, so an unlabelled audit is "no page type", not a type named'').Compatibility
pathPatternstill works and is not deprecated — the two coexist on the same rule and the migration is rule-by-rule. Nothing changes for a deployment that sets nopageTypes.Testing
112 tests pass (3 new), build clean, lint and format clean. New coverage:
pageTypesvalidation, the scoping gate (matches its type / skipped for another / matches any listed / a typeless job matches none), and the job carrying the field through.Sequence
This is PR 2 of 3. #70 (plugin) is PR 1. PR 3 swaps render-service's
pathPattern: '^/product/'forpageTypes: ['pdp']and is blocked on this merging and av1.17.0release tarball.🤖 Generated with Claude Code