Skip to content
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

Capture service worker intercepted requests #1443

Merged

Conversation

nilshah98
Copy link
Contributor

@nilshah98 nilshah98 commented Nov 27, 2023

Changes:

  • Added captureMockedServiceWorker flag under discovery for capturing service worker requests
  • Set bypass: false for Network.setBypassServiceWorker so that service-worker controller can be added and we can access the MSW mocks customer has added.
  • Added serviceWorker parameter, to handleRequests that are not intercepted (since service worker intercepted and will resolve them)

Detailed Changes & Reasoning:

  • Added Network.setBypassServiceWorker to bypass: false so that we can add service-worker controller and access service worker that mocks customer's data.
  • Initially tried to access service-worker requests using Network.getResponseBody, but over here body is returned as "" (empty string) randomly.
    • Tried to look on the internet about why this happens, but no luck :(
  • Finally, decided to call Network.getResponseBody on the original request which service-worker intercepted.
    • Network.getResponseBody is same as what appears in the Response tab in Network section in devtools.
    • For requests that were intercepted by service-worker, the Response tab is always populated.
    • But the service-worker which makes the request upstream or returns mocked data randomly returns body: "" when Network.getResponseBody is called on it.
    • Hence, relying on calling Network.getResponseBody on the original request itself.

Related PRs:
percy/percy-storybook#845

@ninadbstack ninadbstack changed the base branch from master to refactor-network-js November 27, 2023 06:03
@nilshah98 nilshah98 marked this pull request as ready for review November 27, 2023 10:37
@nilshah98 nilshah98 requested a review from a team as a code owner November 27, 2023 10:37
@nilshah98 nilshah98 requested review from prklm10 and pankaj443 and removed request for a team November 27, 2023 10:37
@@ -288,6 +289,7 @@ export function createDiscoveryQueue(percy) {
requestHeaders: snapshot.discovery.requestHeaders,
authorization: snapshot.discovery.authorization,
userAgent: snapshot.discovery.userAgent,
captureServiceWorker: snapshot.discovery.captureServiceWorker,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

per-snapshot level vs global config I'd commented in storybook PR. lets decide what to use and make it uniform.

Copy link
Contributor

@itsjwala itsjwala Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

snapshot.discovery.captureServiceWorker will contain global value, can you verify once?

@@ -114,6 +114,7 @@ function getSnapshotOptions(options, { config, meta }) {
requestHeaders: config.discovery.requestHeaders,
authorization: config.discovery.authorization,
disableCache: config.discovery.disableCache,
captureServiceWorker: config.discovery.captureServiceWorker,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay we're overriding it here so no issues.

Copy link
Contributor

@itsjwala itsjwala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you link locking CLI pr?

@nilshah98
Copy link
Contributor Author

can you link locking CLI pr?

The base branch for this one is https://github.com/percy/cli/tree/refactor-network-js.
Hence, for merging as well, the PR is created for merging into https://github.com/percy/cli/tree/refactor-network-js.

Copy link
Contributor

@Amit3200 Amit3200 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@nilshah98 nilshah98 merged commit 3582916 into refactor-network-js Nov 28, 2023
34 checks passed
@nilshah98 nilshah98 deleted the capture-service-worker-intercepted-requests branch November 28, 2023 11:32
itsjwala pushed a commit that referenced this pull request Nov 29, 2023
#1441)

* feat: added defaultmap + locking mechanism for intercepts queue

* feat: added checkResponseSent lock

* chore: add comments + rename variables

* refactor: move DefaultMap to core/utils

* feat: track lifecycle for ALL requests, does not matter if nterwork is intercepted or not

* feat: add locking mechanism for Network.loadingFailed and Network.eventSourceMessageReceived

* refactor: lock names in requestLifeCycle

* chore: lint fix

* fix: coverage issue with single if blocks

* test: added defaultMap specs

* chore: remove unused intercepts map

* Capture service worker intercepted requests (#1443)

* feat: added new config discovery.captureServiceWorker

* feat: use captureServiceWorker config to capture original requests

* chore: lint fix

* test: added spec for captureServiceWorker

* chore: rename captureServiceWorker -> captureMockedServiceWorker
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants