Skip to content

Commit

Permalink
fix!: typo in 'already-handled' constant of the request interception …
Browse files Browse the repository at this point in the history
…API (#7813)

Issues:  #7745, #7747, #7780
Co-authored-by: Rodrigo Fernández <fdez.romero@gmail.com>
  • Loading branch information
benallfree and FdezRomero committed Dec 9, 2021
1 parent 71cc1b9 commit 8242422
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 39 deletions.
37 changes: 17 additions & 20 deletions docs/api.md
Expand Up @@ -2458,13 +2458,11 @@ Here is the example above rewritten using `request.interceptResolutionState`
```js
/*
This first handler will succeed in calling request.continue because the request interception has never been resolved.
Note: `alreay-handled` is misspelled but likely won't be fixed until v13. https://github.com/puppeteer/puppeteer/pull/7780
*/
page.on('request', (interceptedRequest) => {
// The interception has not been handled yet. Control will pass through this guard.
const { action } = interceptedRequest.interceptResolutionState();
if (action === 'alreay-handled') return;
if (action === InterceptResolutionAction.AlreadyHandled) return;

// It is not strictly necessary to return a promise, but doing so will allow Puppeteer to await this handler.
return new Promise(resolve => {
Expand All @@ -2473,7 +2471,7 @@ page.on('request', (interceptedRequest) => {
// Inside, check synchronously to verify that the intercept wasn't handled already.
// It might have been handled during the 500ms while the other handler awaited an async op of its own.
const { action } = interceptedRequest.interceptResolutionState();
if (action === 'alreay-handled') {
if (action === InterceptResolutionAction.AlreadyHandled) {
resolve();
return;
};
Expand All @@ -2484,12 +2482,12 @@ page.on('request', (interceptedRequest) => {
});
page.on('request', async (interceptedRequest) => {
// The interception has not been handled yet. Control will pass through this guard.
if (interceptedRequest.interceptResolutionState().action === 'alreay-handled') return;
if (interceptedRequest.interceptResolutionState().action === InterceptResolutionAction.AlreadyHandled) return;

await someLongAsyncOperation()
// The interception *MIGHT* have been handled by the first handler, we can't be sure.
// Therefore, we must check again before calling continue() or we risk Puppeteer raising an exception.
if (interceptedRequest.interceptResolutionState().action === 'alreay-handled') return;
if (interceptedRequest.interceptResolutionState().action === InterceptResolutionAction.AlreadyHandled) return;
interceptedRequest.continue();
});
```
Expand Down Expand Up @@ -2547,14 +2545,14 @@ page.on('request', (request) => {

// Control reaches this point because the request was cooperatively aborted which postpones resolution.

// { action: 'abort', priority: 0 }, because abort @ 0 is the current winning resolution
// { action: InterceptResolutionAction.Abort, priority: 0 }, because abort @ 0 is the current winning resolution
console.log(request.interceptResolutionState());

// Legacy Mode: intercept continues immediately.
request.continue({});
});
page.on('request', (request) => {
// { action: 'alreay-handled' }, because continue in Legacy Mode was called
// { action: InterceptResolutionAction.AlreadyHandled }, because continue in Legacy Mode was called
console.log(request.interceptResolutionState());
});

Expand All @@ -2578,7 +2576,7 @@ page.on('request', (request) => {
request.continue(request.continueRequestOverrides(), 5);
});
page.on('request', (request) => {
// { action: 'continue', priority: 5 }, because continue @ 5 > abort @ 0
// { action: InterceptResolutionAction.Continue, priority: 5 }, because continue @ 5 > abort @ 0
console.log(request.interceptResolutionState());
});
```
Expand Down Expand Up @@ -2613,24 +2611,24 @@ page.on('request', (request) => {
request.respond(request.responseForRequest(), 12);
});
page.on('request', (request) => {
// { action: 'respond', priority: 15 }, because respond @ 15 > continue @ 15 > respond @ 12 > abort @ 10
// { action: InterceptResolutionAction.Respond, priority: 15 }, because respond @ 15 > continue @ 15 > respond @ 12 > abort @ 10
console.log(request.interceptResolutionState());
});
```

##### Cooperative Request Continuation

Puppeteer requires `request.continue` to be called explicitly or the request will hang. Even if
your handler means to take no special action, or 'opt out', `request.continue` must still be called.
Puppeteer requires `request.continue()` to be called explicitly or the request will hang. Even if
your handler means to take no special action, or 'opt out', `request.continue()` must still be called.

With the introduction of Cooperative Intercept Mode, two use cases arise for cooperative request continuations:
Unopinionated and Opinionated.

The first case (common) is that your handler means to opt out of doing anything special the request. It has no opinion on further action and simply intends to continue by default and/or defer to other handlers that might have an opinion. But in case there are no other handlers, we must call `request.continue` to ensure that the request doesn't hang.
The first case (common) is that your handler means to opt out of doing anything special the request. It has no opinion on further action and simply intends to continue by default and/or defer to other handlers that might have an opinion. But in case there are no other handlers, we must call `request.continue()` to ensure that the request doesn't hang.

We call this an **Unopinionated continuation** because the intent is to continue the request if nobody else has a better idea. Use `request.continue({...}, DEFAULT_INTERCEPT_RESOLUTION_PRIORITY)` (or `0`) for this type of continuation.

The second case (uncommon) is that your handler actually does have an opinion and means to force continuation by overriding a lower-priority `abort` or `respond` issued elsewhere. We call this an **Opinionated continuation**. In these rare cases where you mean to specify an overriding continuation priority, use a custom priority.
The second case (uncommon) is that your handler actually does have an opinion and means to force continuation by overriding a lower-priority `abort()` or `respond()` issued elsewhere. We call this an **Opinionated continuation**. In these rare cases where you mean to specify an overriding continuation priority, use a custom priority.

To summarize, reason through whether your use of `request.continue` is just meant to be default/bypass behavior vs falling within the intended use case of your handler. Consider using a custom priority for in-scope use cases, and a default priority otherwise. Be aware that your handler may have both Opinionated and Unopinionated cases.

Expand Down Expand Up @@ -5018,8 +5016,7 @@ When in Cooperative Intercept Mode, awaits pending interception handlers and the
#### httpRequest.interceptResolutionState()

- returns: <[InterceptResolutionState]>
- `action` <[InterceptResolutionAction]> Current resolution action. Possible values: `abort`, `respond`, `continue`,
`disabled`, `none`, and `alreay-handled`
- `action` <[InterceptResolutionAction]> Current resolution action.
- `priority` <?[number]> The current priority of the winning action.

`InterceptResolutionAction` is one of:
Expand All @@ -5029,17 +5026,17 @@ When in Cooperative Intercept Mode, awaits pending interception handlers and the
- `continue` - The request will be continued if no higher priority arises.
- `disabled` - Request interception is not currently enabled (see `page.setRequestInterception`).
- `none` - `abort/continue/respond` have not been called yet.
- `alreay-handled` - The interception has already been handled in Legacy Mode by a call to `abort/continue/respond` with
- `already-handled` - The interception has already been handled in Legacy Mode by a call to `abort/continue/respond` with
a `priority` of `undefined`. Subsequent calls to `abort/continue/respond` will throw an exception.

This example will `continue` a request at a slightly higher priority than the current action if the interception has not
This example will `continue()` a request at a slightly higher priority than the current action if the interception has not
already handled and is not already being continued.

```js
page.on('request', (interceptedRequest) => {
const { action, priority } = interceptedRequest.interceptResolutionState();
if (action === 'alreay-handled') return;
if (action === 'continue') return;
if (action === InterceptResolutionAction.AlreadyHandled) return;
if (action === InterceptResolutionAction.Continue) return;

// Change the action to `continue` and bump the priority so `continue` becomes the new winner
interceptedRequest.continue(
Expand Down
47 changes: 30 additions & 17 deletions src/common/HTTPRequest.ts
Expand Up @@ -180,7 +180,7 @@ export class HTTPRequest {
this._frame = frame;
this._redirectChain = redirectChain;
this._continueRequestOverrides = {};
this._interceptResolutionState = { action: 'none' };
this._interceptResolutionState = { action: InterceptResolutionAction.None };
this._interceptHandlers = [];
this._initiator = event.initiator;

Expand Down Expand Up @@ -231,11 +231,13 @@ export class HTTPRequest {
* priority?: number
*
* InterceptResolutionAction is one of: `abort`, `respond`, `continue`,
* `disabled`, `none`, or `alreay-handled`.
* `disabled`, `none`, or `already-handled`.
*/
interceptResolutionState(): InterceptResolutionState {
if (!this._allowInterception) return { action: 'disabled' };
if (this._interceptionHandled) return { action: 'alreay-handled' };
if (!this._allowInterception)
return { action: InterceptResolutionAction.Disabled };
if (this._interceptionHandled)
return { action: InterceptResolutionAction.AlreadyHandled };
return { ...this._interceptResolutionState };
}

Expand Down Expand Up @@ -441,7 +443,10 @@ export class HTTPRequest {
priority > this._interceptResolutionState.priority ||
this._interceptResolutionState.priority === undefined
) {
this._interceptResolutionState = { action: 'continue', priority };
this._interceptResolutionState = {
action: InterceptResolutionAction.Continue,
priority,
};
return;
}
if (priority === this._interceptResolutionState.priority) {
Expand All @@ -451,7 +456,8 @@ export class HTTPRequest {
) {
return;
}
this._interceptResolutionState.action = 'continue';
this._interceptResolutionState.action =
InterceptResolutionAction.Continue;
}
return;
}
Expand Down Expand Up @@ -527,14 +533,17 @@ export class HTTPRequest {
priority > this._interceptResolutionState.priority ||
this._interceptResolutionState.priority === undefined
) {
this._interceptResolutionState = { action: 'respond', priority };
this._interceptResolutionState = {
action: InterceptResolutionAction.Respond,
priority,
};
return;
}
if (priority === this._interceptResolutionState.priority) {
if (this._interceptResolutionState.action === 'abort') {
return;
}
this._interceptResolutionState.action = 'respond';
this._interceptResolutionState.action = InterceptResolutionAction.Respond;
}
}

Expand Down Expand Up @@ -605,7 +614,10 @@ export class HTTPRequest {
priority >= this._interceptResolutionState.priority ||
this._interceptResolutionState.priority === undefined
) {
this._interceptResolutionState = { action: 'abort', priority };
this._interceptResolutionState = {
action: InterceptResolutionAction.Abort,
priority,
};
return;
}
}
Expand All @@ -626,18 +638,19 @@ export class HTTPRequest {
/**
* @public
*/
export type InterceptResolutionAction =
| 'abort'
| 'respond'
| 'continue'
| 'disabled'
| 'none'
| 'alreay-handled';
export enum InterceptResolutionAction {
Abort = 'abort',
Respond = 'respond',
Continue = 'continue',
Disabled = 'disabled',
None = 'none',
AlreadyHandled = 'already-handled',
}

/**
* @public
*
* Deprecate ASAP
* @deprecated please use {@link InterceptResolutionAction} instead.
*/
export type InterceptResolutionStrategy = InterceptResolutionAction;

Expand Down
5 changes: 3 additions & 2 deletions test/requestinterception-experimental.spec.ts
Expand Up @@ -25,6 +25,7 @@ import {
describeFailsFirefox,
} from './mocha-utils'; // eslint-disable-line import/extensions
import { ActionResult } from '../lib/cjs/puppeteer/api-docs-entry.js';
import { InterceptResolutionAction } from '../lib/cjs/puppeteer/common/HTTPRequest.js';

describe('request interception', function () {
setupTestBrowserHooks();
Expand Down Expand Up @@ -845,7 +846,7 @@ describe('request interception', function () {
'Yo, page!'
);
});
it('should indicate alreay-handled if an intercept has been handled', async () => {
it('should indicate already-handled if an intercept has been handled', async () => {
const { page, server } = getTestState();

await page.setRequestInterception(true);
Expand All @@ -857,7 +858,7 @@ describe('request interception', function () {
});
page.on('request', (request) => {
const { action } = request.interceptResolutionState();
expect(action).toBe('alreay-handled');
expect(action).toBe(InterceptResolutionAction.AlreadyHandled);
});
await page.goto(server.EMPTY_PAGE);
});
Expand Down

0 comments on commit 8242422

Please sign in to comment.