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

feat: implement typed events #10889

Merged
merged 1 commit into from Sep 13, 2023
Merged

feat: implement typed events #10889

merged 1 commit into from Sep 13, 2023

Conversation

jrandolf
Copy link
Contributor

@jrandolf jrandolf commented Sep 12, 2023

This PR implements typings for emitted events in all event emitters.

packages/puppeteer-core/src/api/Frame.ts Outdated Show resolved Hide resolved
packages/puppeteer-core/src/api/Page.ts Outdated Show resolved Hide resolved
packages/puppeteer-core/src/api/Page.ts Show resolved Hide resolved
packages/puppeteer-core/src/common/Page.ts Show resolved Hide resolved
packages/puppeteer-core/src/common/Page.ts Show resolved Hide resolved
packages/puppeteer-core/src/common/bidi/Connection.ts Outdated Show resolved Hide resolved
@jrandolf jrandolf force-pushed the jrandolf/refactor-events branch 4 times, most recently from 0e88ac0 to 8660957 Compare September 12, 2023 18:36
@jrandolf jrandolf force-pushed the jrandolf/refactor-events branch 8 times, most recently from 4b5bd0a to d990639 Compare September 12, 2023 20:35
@OrKoN
Copy link
Collaborator

OrKoN commented Sep 12, 2023

Would it be considered a breaking change? I assume if someone used string literals for event names before it might not compile after this change.

@OrKoN
Copy link
Collaborator

OrKoN commented Sep 12, 2023

Could you add a description about what has specifically changed with a basic before and after example? (I assume feat: is marked correctly and indicates a user facing feature and not a refactoring).

@jrandolf
Copy link
Contributor Author

Would it be considered a breaking change? I assume if someone used string literals for event names before it might not compile after this change.

There are no breaking changes, see tests for example.

Could you add a description about what has specifically changed with a basic before and after example? (I assume feat: is marked correctly and indicates a user facing feature and not a refactoring).

Done.

@jrandolf jrandolf force-pushed the jrandolf/refactor-events branch 2 times, most recently from ee9dc8d to 1dedd10 Compare September 12, 2023 21:10
@OrKoN
Copy link
Collaborator

OrKoN commented Sep 12, 2023

Would it be considered a breaking change? I assume if someone used string literals for event names before it might not compile after this change.

There are no breaking changes, see tests for example.

Could you add a description about what has specifically changed with a basic before and after example? (I assume feat: is marked correctly and indicates a user facing feature and not a refactoring).

Done.

so what is the user facing change then? should it be prefixed as refactor:?

@jrandolf
Copy link
Contributor Author

so what is the user facing change then? should it be prefixed as refactor:?

Users now get the correct typing when they try to add a listener which required manual typing previously. We could label it as a refactor if you feel this isn't sufficient to label as a feat.

@OrKoN
Copy link
Collaborator

OrKoN commented Sep 12, 2023

so what is the user facing change then? should it be prefixed as refactor:?

Users now get the correct typing when they try to add a listener which required manual typing previously. We could label it as a refactor if you feel this isn't sufficient to label as a feat.

I am not sure, maybe feat is good. Btw, GitHub UI really struggles to render the diffs in readable manner: I guess it's lazy-loaded so you cannot actually jump to a specific file without scrolling for a while first.

@jrandolf jrandolf force-pushed the jrandolf/refactor-events branch 2 times, most recently from 77001c2 to 1a34f0e Compare September 13, 2023 08:42
packages/puppeteer-core/src/common/FrameManager.ts Outdated Show resolved Hide resolved
packages/puppeteer-core/src/common/FrameManager.ts Outdated Show resolved Hide resolved
packages/puppeteer-core/src/common/bidi/Connection.ts Outdated Show resolved Hide resolved
test/src/bfcache.spec.ts Outdated Show resolved Hide resolved
packages/puppeteer-core/src/common/bidi/Context.ts Outdated Show resolved Hide resolved
@jrandolf jrandolf force-pushed the jrandolf/refactor-events branch 4 times, most recently from 1eddb9d to 069ddb4 Compare September 13, 2023 10:33
Copy link
Collaborator

@Lightning00Blade Lightning00Blade left a comment

Choose a reason for hiding this comment

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

LGTM + please answer the two question like comments and look at nits, and wait for all test to pass.

@jrandolf jrandolf force-pushed the jrandolf/refactor-events branch 4 times, most recently from 6c828e4 to 6331837 Compare September 13, 2023 13:20
@jrandolf jrandolf enabled auto-merge (squash) September 13, 2023 13:26
@jrandolf jrandolf merged commit 9b6f1de into main Sep 13, 2023
26 of 28 checks passed
@jrandolf jrandolf deleted the jrandolf/refactor-events branch September 13, 2023 13:47
This was referenced Sep 13, 2023
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.

None yet

3 participants