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

fix: already-handled spelling [v13] #7813

Merged
merged 30 commits into from Dec 9, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
c944e26
feat: expose intercept resolution state
benallfree Nov 24, 2021
fc2dc2d
chore: comment fix
benallfree Nov 24, 2021
ce3303c
chore: strategy to action
benallfree Nov 25, 2021
e51721e
Update docs/api.md
benallfree Nov 25, 2021
e1bcf80
Update docs/api.md
benallfree Nov 25, 2021
354505e
Update docs/api.md
benallfree Nov 25, 2021
3663436
Update docs/api.md
benallfree Nov 25, 2021
1284dec
Update docs/api.md
benallfree Nov 25, 2021
44b2cf3
Update docs/api.md
benallfree Nov 25, 2021
91abf11
chore: formatting
benallfree Nov 25, 2021
4e5dd2b
Merge branch 'expose-intercept-resolution-2' of github.com:benallfree…
benallfree Nov 25, 2021
a191f95
chore: rename
benallfree Nov 25, 2021
bf183bf
chore: docs
benallfree Nov 25, 2021
54c99ce
chore: docs
benallfree Nov 25, 2021
942bdff
chore: docs & default priority
benallfree Nov 25, 2021
53ddc0a
chore: typos
benallfree Nov 25, 2021
ae4b5c9
chore: typo
benallfree Nov 25, 2021
500b8cb
chore: typo
benallfree Nov 25, 2021
541d2b8
Merge branch 'main' into expose-intercept-resolution-2
benallfree Nov 29, 2021
98e2814
chore: revert alreay-handled
benallfree Nov 29, 2021
5d7d7e7
Merge branch 'expose-intercept-resolution-2' of github.com:benallfree…
benallfree Nov 29, 2021
70969e9
fix: already-handled spelling error
benallfree Nov 29, 2021
ec49e8a
Merge branch 'main' into already-handled-fix-v13
benallfree Dec 3, 2021
2a84f64
Merge branch 'main' into already-handled-fix-v13
benallfree Dec 3, 2021
5a0a422
Merge branch 'upstream-main' into already-handled-fix-v13
benallfree Dec 7, 2021
afa8c3f
Merge branch 'already-handled-fix-v13' of github.com:benallfree/puppe…
benallfree Dec 7, 2021
f9f96dc
Merge branch 'main' into already-handled-fix-v13
benallfree Dec 7, 2021
e472bf9
chore: fix d.ts import
benallfree Dec 7, 2021
d06d5da
chore: typo
benallfree Dec 9, 2021
a1e0f37
Merge branch 'main' into already-handled-fix-v13
OrKoN Dec 9, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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