[Defer Uploads v2] Added support for handling multiple root resource in single asset discovery phase #1728
Conversation
…in single asset discovery phase
packages/core/src/discovery.js
Outdated
| let allResources = new Set(); | ||
|
|
||
| if (!Array.isArray(domSnapshot)) { | ||
| domSnapshot = [{ domSnapshot }]; |
There was a problem hiding this comment.
should this be an object array with domSnapshot key i it ? that doesnt seem to match your schema. It should be domSnapshot = [ domSnapshot, ]; as per schema
There was a problem hiding this comment.
This is correct. check schema
There was a problem hiding this comment.
Any specific reason to put width and domSnapshot separately ? shouldnt width be part of it ?
Especially considering you are defining it in schema separately but not passing it here on line 97 ? if its optional it would work with domSnapshot as well will keep connected items together
packages/core/src/discovery.js
Outdated
| if (discovery.captureResponsiveAssetsEnabled) { forceReload = true; } | ||
| yield page.goto(snapshot.url, { cookies, forceReload }); |
There was a problem hiding this comment.
Optional
| if (discovery.captureResponsiveAssetsEnabled) { forceReload = true; } | |
| yield page.goto(snapshot.url, { cookies, forceReload }); | |
| yield page.goto(snapshot.url, { cookies, forceReload: discovery.captureResponsiveAssetsEnabled }); |
There was a problem hiding this comment.
@chinmay-browserstack also account for the additional cookies which we are planning to capture via SDKs, how they are going to be passed here?
There was a problem hiding this comment.
we can pass cookies is same fashion as we currently do inside domSnapshot object. we will override this value
| 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.
Do we not need to wait for navigation here ? does it work regardless ?
There was a problem hiding this comment.
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.
have we added tests for this ? or ^ is from chromium documentation ?
packages/core/src/config.js
Outdated
| @@ -291,6 +295,7 @@ export const snapshotSchema = { | |||
| domTransformation: { $ref: '/config/snapshot#/properties/domTransformation' }, | |||
| enableLayout: { $ref: '/config/snapshot#/properties/enableLayout' }, | |||
| sync: { $ref: '/config/snapshot#/properties/sync' }, | |||
| multiDOM: { $ref: '/config/snapshot#/properties/multiDOM' }, | |||
There was a problem hiding this comment.
Doesnt sound very straightforward for end user. captureMultipleWidths seems better - it does exactly what it says.
| return page.resize({ | ||
| height: snapshot.minHeight, | ||
| deviceScaleFactor, | ||
| mobile, | ||
| width | ||
| }); |
There was a problem hiding this comment.
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.
Actually page class is a wrapper only for running different cdp commands on browser.
| @@ -365,8 +400,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.
is this responsible for returning the resource for that width?
If width is not present what are we planning to do, I am seeing we are returning first index are we sure its works?
There was a problem hiding this comment.
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
packages/core/src/network.js
Outdated
| @@ -354,7 +354,7 @@ async function sendResponseResource(network, request, session) { | |||
| let send = (method, params) => network.send(session, method, params); | |||
|
|
|||
| try { | |||
| let resource = network.intercept.getResource(url); | |||
| let resource = network.intercept.getResource(url, network.currentWidth); | |||
There was a problem hiding this comment.
apart from network.currentWidth is there any way to get any width this looks little weird.
There was a problem hiding this comment.
we can use cdp to get current window width but that will be slower operation as compared to this.
There was a problem hiding this comment.
I have commented to make it better
packages/core/src/discovery.js
Outdated
| width | ||
| }); | ||
| let resizePage = width => { | ||
| page.network.currentWidth = width; |
There was a problem hiding this comment.
Network is not directly related to current width. Dont update in network. Update in intercept instead as intercept needs current width to save/retrieve resources. Network just passes it to intercept in the end
| @@ -62,6 +62,12 @@ export function createPercyServer(percy, port) { | |||
| build: percy.testing?.build ?? percy.build, | |||
| loglevel: percy.loglevel(), | |||
| config: percy.config, | |||
| widths: { | |||
| // This is always needed even if width is passed | |||
| mobile: percy.deviceDetails ? percy.deviceDetails.map((d) => d.width) : [], | |||
There was a problem hiding this comment.
Shouldnt we send dpr as well ?
There was a problem hiding this comment.
doesn't make sense as we are just resizing page and not reloading page so changing dpr wouldn't help
multiDOMwhich when passed we capture DOM in all widths in which it will be rendered.domSnapshot. now domSnapshot can also be a list of objects with following strucuture{ 'domSnapshot': <DOM>, 'width': <int>}getResourcewill return root resource based on width on which discovery browser is opened. in case root resource not present for that width we will fallback to first dom in list.captureSrcsetif multiDOM is enabled.