fix: type page event listeners correctly #6891
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jackfranklin
force-pushed
the
page-event-object
branch
from
abefe64
to
8333d04
Compare
mathiasbynens
approved these changes
Mar 24, 2021
jschfflr
reviewed
Mar 24, 2021
src/common/HTTPRequest.ts
Outdated
| /** | ||
| * Optional response headers. All values are converted to strings. | ||
| */ | ||
| headers: Record<string, any>; |
There was a problem hiding this comment.
Why did you change this to any?
There was a problem hiding this comment.
Good point, not sure. I think we stringify them anyway so unknown is better here.
| requestfailed: HTTPRequest; | ||
| requestfinished: HTTPRequest; | ||
| workercreated: WebWorker; | ||
| workerdestroyed: WebWorker; |
There was a problem hiding this comment.
What about adding a [event: string]: any at the bottom here to make sure we don't break places that use other event names? Or is that intended?
There was a problem hiding this comment.
I'd rather it fail explicitly because we're missing a type.
This PR fixes the fact that currently if you have:
```ts
page.on('request', request => {
})
```
Then `request` will be typed as `any`. We can fix this by defining an
interface of event name => callback argument type, and looking that up
when you call `page.on`.
Also includes a drive-by fix to ensure we convert response headers to
strings, and updates the types accordingly.
jackfranklin
force-pushed
the
page-event-object
branch
from
f2c26d3
to
2f69f52
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes the fact that currently if you have:
Then
requestwill be typed asany. We can fix this by defining aninterface of event name => callback argument type, and looking that up
when you call
page.on.Fixes #6854.