-
Notifications
You must be signed in to change notification settings - Fork 43
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
[Defer Uploads v2] Added support for handling multiple root resource in single asset discovery phase #1728
base: master
Are you sure you want to change the base?
[Defer Uploads v2] Added support for handling multiple root resource in single asset discovery phase #1728
Changes from all commits
8ee6198
fb2afde
9c25c5b
539c49b
78c7678
ebed72a
d487b6a
70f370f
d7de0d9
fc065a2
af0989e
dffcee0
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 |
---|---|---|
|
@@ -88,21 +88,51 @@ function waitForDiscoveryNetworkIdle(page, options) { | |
|
||
// Creates an initial resource map for a snapshot containing serialized DOM | ||
function parseDomResources({ url, domSnapshot }) { | ||
if (!domSnapshot) return new Map(); | ||
let isHTML = typeof domSnapshot === 'string'; | ||
let { html, resources = [] } = isHTML ? { html: domSnapshot } : domSnapshot; | ||
let rootResource = createRootResource(url, html); | ||
const map = new Map(); | ||
if (!domSnapshot) return map; | ||
let allRootResources = new Set(); | ||
let allResources = new Set(); | ||
|
||
if (!Array.isArray(domSnapshot)) { | ||
domSnapshot = [domSnapshot]; | ||
} | ||
|
||
for (let dom of domSnapshot) { | ||
let isHTML = typeof dom === 'string'; | ||
let { html, resources = [] } = isHTML ? { html: dom } : dom; | ||
resources.forEach(r => allResources.add(r)); | ||
const attrs = dom.width ? { widths: [dom.width] } : {}; | ||
let rootResource = createRootResource(url, html, attrs); | ||
allRootResources.add(rootResource); | ||
} | ||
allRootResources = Array.from(allRootResources); | ||
map.set(allRootResources[0].url, allRootResources); | ||
allResources = Array.from(allResources); | ||
|
||
// reduce the array of resources into a keyed map | ||
return resources.reduce((map, { url, content, mimetype }) => { | ||
return allResources.reduce((map, { url, content, mimetype }) => { | ||
ninadbstack marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// serialized resource contents are base64 encoded | ||
content = Buffer.from(content, mimetype.includes('text') ? 'utf8' : 'base64'); | ||
// specify the resource as provided to prevent overwriting during asset discovery | ||
let resource = createResource(url, content, mimetype, { provided: true }); | ||
// key the resource by its url and return the map | ||
return map.set(resource.url, resource); | ||
// the initial map is created with at least a root resource | ||
}, new Map([[rootResource.url, rootResource]])); | ||
}, map); | ||
} | ||
|
||
function createAndApplyPercyCSS({ percyCSS, roots }) { | ||
let css = createPercyCSSResource(roots[0].url, percyCSS); | ||
|
||
// replace root contents and associated properties | ||
roots.forEach(root => { | ||
Object.assign(root, createRootResource(root.url, ( | ||
root.content.replace(/(<\/body>)(?!.*\1)/is, ( | ||
`<link data-percy-specific-css rel="stylesheet" href="${css.pathname}"/>` | ||
) + '$&')))); | ||
}); | ||
|
||
return css; | ||
} | ||
|
||
// Calls the provided callback with additional resources | ||
|
@@ -111,14 +141,14 @@ function processSnapshotResources({ domSnapshot, resources, ...snapshot }) { | |
resources = [...(resources?.values() ?? [])]; | ||
|
||
// find any root resource matching the provided dom snapshot | ||
let rootContent = domSnapshot?.html ?? domSnapshot; | ||
let root = resources.find(r => r.content === rootContent); | ||
// since root resources are stored as array | ||
let roots = resources.find(r => Array.isArray(r)); | ||
|
||
// initialize root resources if needed | ||
if (!root) { | ||
if (!roots) { | ||
let domResources = parseDomResources({ ...snapshot, domSnapshot }); | ||
resources = [...domResources.values(), ...resources]; | ||
root = resources[0]; | ||
roots = resources.find(r => Array.isArray(r)); | ||
} | ||
|
||
// inject Percy CSS | ||
|
@@ -129,16 +159,13 @@ function processSnapshotResources({ domSnapshot, resources, ...snapshot }) { | |
log.warn('DOM elements found outside </body>, percyCSS might not work'); | ||
} | ||
|
||
let css = createPercyCSSResource(root.url, snapshot.percyCSS); | ||
ninadbstack marked this conversation as resolved.
Show resolved
Hide resolved
|
||
resources.push(css); | ||
|
||
// replace root contents and associated properties | ||
Object.assign(root, createRootResource(root.url, ( | ||
root.content.replace(/(<\/body>)(?!.*\1)/is, ( | ||
`<link data-percy-specific-css rel="stylesheet" href="${css.pathname}"/>` | ||
) + '$&')))); | ||
const percyCSSReource = createAndApplyPercyCSS({ percyCSS: snapshot.percyCSS, roots }); | ||
resources.push(percyCSSReource); | ||
} | ||
|
||
// For multi dom root resources are stored as array | ||
resources = resources.flat(); | ||
|
||
// include associated snapshot logs matched by meta information | ||
resources.push(createLogResource(logger.query(log => ( | ||
log.meta.snapshot?.testCase === snapshot.meta.snapshot.testCase && log.meta.snapshot?.name === snapshot.meta.snapshot.name | ||
|
@@ -195,16 +222,19 @@ async function* captureSnapshotResources(page, snapshot, options) { | |
}; | ||
|
||
// used to resize the using capture options | ||
let resizePage = width => page.resize({ | ||
height: snapshot.minHeight, | ||
deviceScaleFactor, | ||
mobile, | ||
width | ||
}); | ||
let resizePage = width => { | ||
page.network.intercept.currentWidth = width; | ||
return page.resize({ | ||
height: snapshot.minHeight, | ||
deviceScaleFactor, | ||
mobile, | ||
width | ||
}); | ||
Comment on lines
+227
to
+232
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. Generally if we are having too many CDP commands shouldn't we consider creating a wrapper class which will receive parameters and do the things using that? 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. Actually page class is a wrapper only for running different cdp commands on browser. |
||
}; | ||
|
||
// navigate to the url | ||
yield resizePage(snapshot.widths[0]); | ||
yield page.goto(snapshot.url, { cookies }); | ||
yield page.goto(snapshot.url, { cookies, forceReload: discovery.captureResponsiveAssetsEnabled }); | ||
|
||
// wait for any specified timeout | ||
if (snapshot.discovery.waitForTimeout && page.enableJavaScript) { | ||
|
@@ -228,7 +258,8 @@ async function* captureSnapshotResources(page, snapshot, options) { | |
// Running before page idle since this will trigger many network calls | ||
// so need to run as early as possible. plus it is just reading urls from dom srcset | ||
// which will be already loaded after navigation complete | ||
if (discovery.captureSrcset) { | ||
// Don't run incase of responsiveSnapshotCapture since we are running discovery for all widths so images will get captured in all required widths | ||
if (!snapshot.responsiveSnapshotCapture && discovery.captureSrcset) { | ||
await page.insertPercyDom(); | ||
yield page.eval('window.PercyDOM.loadAllSrcsetLinks()'); | ||
} | ||
|
@@ -247,6 +278,7 @@ async function* captureSnapshotResources(page, snapshot, options) { | |
yield page.evaluate(execute?.beforeResize); | ||
yield waitForDiscoveryNetworkIdle(page, discovery); | ||
yield resizePage(width = widths[i + 1]); | ||
if (snapshot.responsiveSnapshotCapture) { yield page.goto(snapshot.url, { cookies, forceReload: true }); } | ||
yield page.evaluate(execute?.afterResize); | ||
} | ||
} | ||
|
@@ -365,8 +397,15 @@ export function createDiscoveryQueue(percy) { | |
disableCache: snapshot.discovery.disableCache, | ||
allowedHostnames: snapshot.discovery.allowedHostnames, | ||
disallowedHostnames: snapshot.discovery.disallowedHostnames, | ||
getResource: u => snapshot.resources.get(u) || cache.get(u), | ||
saveResource: r => { snapshot.resources.set(r.url, r); if (!r.root) { cache.set(r.url, r); } } | ||
getResource: (u, width = null) => { | ||
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. is this responsible for returning the resource for that width? 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. Yes it works. since only root resource is an array and in case if multiDOM is disabled we are storing root resource as a array only with no widths. that case is working fine. for other resources no change in logic |
||
let resource = snapshot.resources.get(u) || cache.get(u); | ||
if (resource && Array.isArray(resource) && resource[0].root) { | ||
const rootResource = resource.find(r => r.widths?.includes(width)); | ||
resource = rootResource || resource[0]; | ||
} | ||
return resource; | ||
}, | ||
saveResource: r => { snapshot.resources.set(r.url, r); cache.set(r.url, r); } | ||
} | ||
}); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,9 +68,14 @@ export class Page { | |
} | ||
|
||
// Go to a URL and wait for navigation to occur | ||
async goto(url, { waitUntil = 'load', cookies } = {}) { | ||
async goto(url, { waitUntil = 'load', cookies, forceReload } = {}) { | ||
this.log.debug(`Navigate to: ${url}`, this.meta); | ||
|
||
if (forceReload) { | ||
this.log.debug('Navigating to blank page', this.meta); | ||
await this.session.send('Page.navigate', { url: 'about:blank' }); | ||
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. Do we not need to wait for navigation here ? does it work regardless ? 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. about:blank is an empty page and does not load any external resources so i don't think we need to wait for it to load. it is instantaneous 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. have we added tests for this ? or ^ is from chromium documentation ? |
||
} | ||
|
||
let navigate = async () => { | ||
const userPassedCookie = this.session.browser.cookies; | ||
// set cookies before navigation so we can default the domain to this hostname | ||
|
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.
Shouldnt we send dpr as well ?
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.
doesn't make sense as we are just resizing page and not reloading page so changing dpr wouldn't help