From 23877c4419530fcd76cb49746bca22336c55fe8f Mon Sep 17 00:00:00 2001 From: Steven Kitterman Date: Fri, 28 Jun 2024 09:41:41 -0700 Subject: [PATCH 1/7] Don't rebufferize archived content since we already buffer it in Watcher --- packages/cypress/src/index.ts | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/packages/cypress/src/index.ts b/packages/cypress/src/index.ts index 2f80ceba..b01a1e87 100644 --- a/packages/cypress/src/index.ts +++ b/packages/cypress/src/index.ts @@ -35,19 +35,6 @@ const writeArchives = async ({ pageUrl, viewport, }: WriteArchivesParams) => { - const bufferedArchiveList = Object.entries(resourceArchive).map(([key, value]) => { - return [ - key, - { - ...value, - // we can't use Buffer in the browser (when we collect the responses) - // so we go through one by one here and bufferize them - // @ts-expect-error will fix when Cypress has its own package - body: Buffer.from(value.body, 'utf8'), - }, - ]; - }); - const allSnapshots = Object.fromEntries( // manual snapshots can be given a name; otherwise, just use the snapshot's place in line as the name domSnapshots.map(({ name, snapshot }, index) => [ @@ -66,7 +53,7 @@ const writeArchives = async ({ viewport, }, allSnapshots, - Object.fromEntries(bufferedArchiveList), + resourceArchive, chromaticStorybookParams ); }; From 53de9acbe67a018c4655342dd5955ae0ec16bf83 Mon Sep 17 00:00:00 2001 From: Steven Kitterman Date: Fri, 28 Jun 2024 09:49:35 -0700 Subject: [PATCH 2/7] Watcher renamed to ResourceArchiver to be consistent with file name and to describe what it does more clearly --- packages/cypress/src/index.ts | 18 +++++++++--------- .../playwright/src/createResourceArchive.ts | 8 ++++---- packages/shared/src/resource-archive/index.ts | 2 +- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/packages/cypress/src/index.ts b/packages/cypress/src/index.ts index b01a1e87..2d09feb0 100644 --- a/packages/cypress/src/index.ts +++ b/packages/cypress/src/index.ts @@ -1,7 +1,7 @@ import type { elementNode } from 'rrweb-snapshot'; import CDP, { Version } from 'chrome-remote-interface'; import { - Watcher, + ResourceArchiver, writeTestResult, ChromaticStorybookParameters, ResourceArchive, @@ -58,11 +58,11 @@ const writeArchives = async ({ ); }; -// using a single Watcher instance across all tests (for the test run) +// using a single ResourceArchiver instance across all tests (for the test run) // each time a test completes, we'll save to disk whatever archives are there at that point. // This should be safe since the same resource from the same URL should be the same during the entire test run. // Cypress doesn't give us a way to share variables between the "before test" and "after test" lifecycle events on the server. -let watcher: Watcher = null; +let resourceArchiver: ResourceArchiver = null; let host = ''; let port = 0; @@ -82,9 +82,9 @@ const setupNetworkListener = async ({ target: webSocketDebuggerUrl, }); - if (!watcher) { - watcher = new Watcher(cdp, allowedDomains); - await watcher.watch(); + if (!resourceArchiver) { + resourceArchiver = new ResourceArchiver(cdp, allowedDomains); + await resourceArchiver.watch(); } } catch (err) { console.log('err', err); @@ -95,11 +95,11 @@ const setupNetworkListener = async ({ const saveArchives = (archiveInfo: WriteParams) => { return new Promise((resolve) => { - // the watcher's archives come from the server, everything else (DOM snapshots, test info, etc) comes from the browser - // notice we're not calling + awaiting watcher.idle() here... + // the resourceArchiver's archives come from the server, everything else (DOM snapshots, test info, etc) comes from the browser + // notice we're not calling + awaiting resourceArchiver.idle() here... // that's because in Cypress, cy.visit() waits until all resources have loaded before finishing // so at this point (after the test) we're confident that the resources are all there already without having to wait more - return writeArchives({ ...archiveInfo, resourceArchive: watcher.archive }).then(() => { + return writeArchives({ ...archiveInfo, resourceArchive: resourceArchiver.archive }).then(() => { resolve(null); }); }); diff --git a/packages/playwright/src/createResourceArchive.ts b/packages/playwright/src/createResourceArchive.ts index 45553f86..b35bf84d 100644 --- a/packages/playwright/src/createResourceArchive.ts +++ b/packages/playwright/src/createResourceArchive.ts @@ -1,6 +1,6 @@ import type { Page } from 'playwright'; import { - Watcher, + ResourceArchiver, ResourceArchive, DEFAULT_GLOBAL_RESOURCE_ARCHIVE_TIMEOUT_MS, logger, @@ -43,12 +43,12 @@ export const createResourceArchive = async ({ }): Promise<() => Promise> => { const cdpClient = await page.context().newCDPSession(page); - const watcher = new Watcher(cdpClient, assetDomains); - await watcher.watch(); + const resourceArchiver = new ResourceArchiver(cdpClient, assetDomains); + await resourceArchiver.watch(); return async () => { await idle(page, networkTimeout ?? DEFAULT_GLOBAL_RESOURCE_ARCHIVE_TIMEOUT_MS); - return watcher.archive; + return resourceArchiver.archive; }; }; diff --git a/packages/shared/src/resource-archive/index.ts b/packages/shared/src/resource-archive/index.ts index 27a722b4..98b180c0 100644 --- a/packages/shared/src/resource-archive/index.ts +++ b/packages/shared/src/resource-archive/index.ts @@ -22,7 +22,7 @@ interface CDPClient { send: (eventName: keyof Protocol.CommandParameters, payload?: any) => Promise; } -export class Watcher { +export class ResourceArchiver { public archive: ResourceArchive = {}; private client: CDPClient; From c54be809dd4844f355db38902659be3318393433 Mon Sep 17 00:00:00 2001 From: Steven Kitterman Date: Fri, 28 Jun 2024 09:50:25 -0700 Subject: [PATCH 3/7] File name tweaked to match exported class name exactly --- packages/shared/src/index.ts | 2 +- .../shared/src/{resource-archive => resource-archiver}/index.ts | 0 packages/shared/src/write-archive/archive-file.test.ts | 2 +- packages/shared/src/write-archive/archive-file.ts | 2 +- packages/shared/src/write-archive/index.ts | 2 +- 5 files changed, 4 insertions(+), 4 deletions(-) rename packages/shared/src/{resource-archive => resource-archiver}/index.ts (100%) diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index 649b386d..45a2c428 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -1,4 +1,4 @@ -export * from './resource-archive'; +export * from './resource-archiver'; export * from './write-archive'; export * from './utils/analytics'; export * from './utils/logger'; diff --git a/packages/shared/src/resource-archive/index.ts b/packages/shared/src/resource-archiver/index.ts similarity index 100% rename from packages/shared/src/resource-archive/index.ts rename to packages/shared/src/resource-archiver/index.ts diff --git a/packages/shared/src/write-archive/archive-file.test.ts b/packages/shared/src/write-archive/archive-file.test.ts index 92966dce..7e00f3b6 100644 --- a/packages/shared/src/write-archive/archive-file.test.ts +++ b/packages/shared/src/write-archive/archive-file.test.ts @@ -1,5 +1,5 @@ import { ArchiveFile } from './archive-file'; -import type { ArchiveResponse, UrlString } from '../resource-archive'; +import type { ArchiveResponse, UrlString } from '../resource-archiver'; const imgPng = 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNkYAAAAAYAAjCB0C8AAAAASUVORK5CYII='; diff --git a/packages/shared/src/write-archive/archive-file.ts b/packages/shared/src/write-archive/archive-file.ts index 0174e335..705e86ba 100644 --- a/packages/shared/src/write-archive/archive-file.ts +++ b/packages/shared/src/write-archive/archive-file.ts @@ -2,7 +2,7 @@ import mime from 'mime'; import path from 'path'; import { createHash } from 'node:crypto'; import { logger } from '../utils/logger'; -import type { ArchiveResponse, UrlString } from '../resource-archive'; +import type { ArchiveResponse, UrlString } from '../resource-archiver'; const RESERVED_PATHNAMES: string[] = [ // This is a list of filenames that are reserved for Storybook as a dev target. diff --git a/packages/shared/src/write-archive/index.ts b/packages/shared/src/write-archive/index.ts index d5af9c10..cc703da5 100644 --- a/packages/shared/src/write-archive/index.ts +++ b/packages/shared/src/write-archive/index.ts @@ -3,7 +3,7 @@ import { outputFile, ensureDir, outputJSONFile } from '../utils/filePaths'; import { logger } from '../utils/logger'; import { ArchiveFile } from './archive-file'; import { DOMSnapshot } from './dom-snapshot'; -import type { ResourceArchive } from '../resource-archive'; +import type { ResourceArchive } from '../resource-archiver'; import type { ChromaticStorybookParameters } from '../types'; import { Viewport } from '../utils/viewport'; import { snapshotFileName, snapshotId } from './snapshot-files'; From c65e5734ae0da3013235c02cf2dbf366c4e0d68c Mon Sep 17 00:00:00 2001 From: Steven Kitterman Date: Fri, 28 Jun 2024 10:17:21 -0700 Subject: [PATCH 4/7] Code decomplexified a bit --- .../shared/src/resource-archiver/index.ts | 84 ++++++++++++------- 1 file changed, 52 insertions(+), 32 deletions(-) diff --git a/packages/shared/src/resource-archiver/index.ts b/packages/shared/src/resource-archiver/index.ts index 98b180c0..729be30d 100644 --- a/packages/shared/src/resource-archiver/index.ts +++ b/packages/shared/src/resource-archiver/index.ts @@ -110,40 +110,15 @@ export class ResourceArchiver { // Pausing a response stage with a response if (responseStatusCode) { - if ([301, 302, 307, 308].includes(responseStatusCode)) { - await this.clientSend(request, 'Fetch.continueRequest', { - requestId, - interceptResponse: true, - }); - return; - } - - const result = await this.clientSend(request, 'Fetch.getResponseBody', { + await this.handleSuccessfulResponse( + request, requestId, - }); - // Something has gone wrong and will be logged above - if (result === null) { - return; - } - const { body, base64Encoded } = result; - - // If the Content-Type header is present, let's capture it. - const contentTypeHeader: Protocol.Fetch.HeaderEntry = responseHeaders.find( - ({ name }) => name.toLowerCase() === 'content-type' + responseStatusCode, + responseStatusText, + responseHeaders, + requestUrl, + isRequestFromAllowedDomain ); - - // No need to capture the response of the top level page request - const isFirstRequest = requestUrl.toString() === this.firstUrl.toString(); - if (isRequestFromAllowedDomain && !isFirstRequest) { - this.archive[request.url] = { - statusCode: responseStatusCode, - statusText: responseStatusText, - body: Buffer.from(body, base64Encoded ? 'base64' : 'utf8'), - contentType: contentTypeHeader?.value, - }; - } - - await this.clientSend(request, 'Fetch.continueRequest', { requestId }); return; } @@ -152,4 +127,49 @@ export class ResourceArchiver { interceptResponse: true, }); } + + private async handleSuccessfulResponse( + request: Protocol.Fetch.requestPausedPayload['request'], + requestId: Protocol.Fetch.requestPausedPayload['requestId'], + responseStatusCode: Protocol.Fetch.requestPausedPayload['responseStatusCode'], + responseStatusText: Protocol.Fetch.requestPausedPayload['responseStatusText'], + responseHeaders: Protocol.Fetch.requestPausedPayload['responseHeaders'], + requestUrl: URL, + isRequestFromAllowedDomain: boolean + ) { + if ([301, 302, 307, 308].includes(responseStatusCode)) { + await this.clientSend(request, 'Fetch.continueRequest', { + requestId, + interceptResponse: true, + }); + return; + } + + const result = await this.clientSend(request, 'Fetch.getResponseBody', { + requestId, + }); + // Something has gone wrong and will be logged above + if (result === null) { + return; + } + const { body, base64Encoded } = result; + + // If the Content-Type header is present, let's capture it. + const contentTypeHeader: Protocol.Fetch.HeaderEntry = responseHeaders.find( + ({ name }) => name.toLowerCase() === 'content-type' + ); + + // No need to capture the response of the top level page request + const isFirstRequest = requestUrl.toString() === this.firstUrl.toString(); + if (isRequestFromAllowedDomain && !isFirstRequest) { + this.archive[request.url] = { + statusCode: responseStatusCode, + statusText: responseStatusText, + body: Buffer.from(body, base64Encoded ? 'base64' : 'utf8'), + contentType: contentTypeHeader?.value, + }; + } + + await this.clientSend(request, 'Fetch.continueRequest', { requestId }); + } } From 72c4ec0bf5149c0300d5192930791f58c1624d4e Mon Sep 17 00:00:00 2001 From: Steven Kitterman Date: Fri, 28 Jun 2024 10:21:57 -0700 Subject: [PATCH 5/7] Object used for params instead of tons of separate params --- .../shared/src/resource-archiver/index.ts | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/packages/shared/src/resource-archiver/index.ts b/packages/shared/src/resource-archiver/index.ts index 729be30d..ae4e4cfc 100644 --- a/packages/shared/src/resource-archiver/index.ts +++ b/packages/shared/src/resource-archiver/index.ts @@ -111,11 +111,13 @@ export class ResourceArchiver { // Pausing a response stage with a response if (responseStatusCode) { await this.handleSuccessfulResponse( - request, - requestId, - responseStatusCode, - responseStatusText, - responseHeaders, + { + request, + requestId, + responseStatusCode, + responseStatusText, + responseHeaders, + }, requestUrl, isRequestFromAllowedDomain ); @@ -129,14 +131,16 @@ export class ResourceArchiver { } private async handleSuccessfulResponse( - request: Protocol.Fetch.requestPausedPayload['request'], - requestId: Protocol.Fetch.requestPausedPayload['requestId'], - responseStatusCode: Protocol.Fetch.requestPausedPayload['responseStatusCode'], - responseStatusText: Protocol.Fetch.requestPausedPayload['responseStatusText'], - responseHeaders: Protocol.Fetch.requestPausedPayload['responseHeaders'], + requestPausedPayload: Pick< + Protocol.Fetch.requestPausedPayload, + 'request' | 'requestId' | 'responseStatusCode' | 'responseStatusText' | 'responseHeaders' + >, requestUrl: URL, isRequestFromAllowedDomain: boolean ) { + const { request, requestId, responseStatusCode, responseStatusText, responseHeaders } = + requestPausedPayload; + if ([301, 302, 307, 308].includes(responseStatusCode)) { await this.clientSend(request, 'Fetch.continueRequest', { requestId, From 13b56be1c697aaf134dcab6199aa7c1496350256 Mon Sep 17 00:00:00 2001 From: Steven Kitterman Date: Fri, 28 Jun 2024 10:40:07 -0700 Subject: [PATCH 6/7] Changeset created --- .changeset/friendly-avocados-hammer.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/friendly-avocados-hammer.md diff --git a/.changeset/friendly-avocados-hammer.md b/.changeset/friendly-avocados-hammer.md new file mode 100644 index 00000000..5ba08308 --- /dev/null +++ b/.changeset/friendly-avocados-hammer.md @@ -0,0 +1,7 @@ +--- +'@chromatic-com/playwright': patch +'@chromatic-com/cypress': patch +'@chromatic-com/shared-e2e': patch +--- + +Pass Cypress archive along instead of re-bufferizing; refactored resource-archiving code slightly From 1702f2194e44f809af4fceba91094c24ff16a01d Mon Sep 17 00:00:00 2001 From: Steven Kitterman Date: Fri, 28 Jun 2024 15:28:58 -0700 Subject: [PATCH 7/7] temp empty