Skip to content

refactor: use stricter CDP implementation type of CDPSession - #13620

Merged
Lightning00Blade merged 1 commit into
puppeteer:mainfrom
alexey-pelykh:imp/cdp-FrameManager-typing
Feb 19, 2025
Merged

refactor: use stricter CDP implementation type of CDPSession#13620
Lightning00Blade merged 1 commit into
puppeteer:mainfrom
alexey-pelykh:imp/cdp-FrameManager-typing

Conversation

@alexey-pelykh

Copy link
Copy Markdown
Contributor

What kind of change does this PR introduce?

Tiny typing improvement.

Did you add tests for your changes?

There's no functional change

If relevant, did you update the documentation?

No - nothing affected

Summary

As a part of #13613 there were few typing changes so that CDP FrameManager uses CDP-specific session.

Does this PR introduce a breaking change?

Nope

Other information

N/A

@Lightning00Blade Lightning00Blade left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I would prefer to not include a runtime checks when this is internal code, that users won't ever interact with, TypeScript safety should be enough for internal parts.
(I know that there was already one, but that should not be there as well).

I looked into changing the types, only and it looked like it was failing in a couple of places due to changing the constructor. And without that change other places don't benefit as much with this change.

@alexey-pelykh
alexey-pelykh force-pushed the imp/cdp-FrameManager-typing branch from 4c769ac to 426d7a2 Compare February 17, 2025 14:44
@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

@Lightning00Blade thanks for the review! Your points are perfectly valid - I've tried to avoid runtime checks and gradually replace non-CDP-specific types with CDP-specific ones and eventually got success, at least it looks like it.

Comment thread packages/puppeteer-core/src/cdp/Connection.ts
@Lightning00Blade Lightning00Blade changed the title imp: cdp/FrameManager typing refactor: use stricter CDP implementation type of CDPSession Feb 17, 2025

@Lightning00Blade Lightning00Blade left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the PR, left some comments.

Comment thread packages/puppeteer-core/src/cdp/Target.ts
Comment thread packages/puppeteer-core/src/cdp/Target.ts Outdated
Comment thread packages/puppeteer-core/src/cdp/Page.ts Outdated
Comment thread docs/api/puppeteer.connection.createsession.md
Comment thread docs/api/puppeteer.connection.session.md
@alexey-pelykh
alexey-pelykh force-pushed the imp/cdp-FrameManager-typing branch 4 times, most recently from dbd6750 to e4003c7 Compare February 17, 2025 17:59
@alexey-pelykh
alexey-pelykh requested a review from OrKoN February 17, 2025 18:00
Comment thread packages/puppeteer-core/src/cdp/Page.ts
@alexey-pelykh
alexey-pelykh force-pushed the imp/cdp-FrameManager-typing branch from e4003c7 to ccb9f41 Compare February 17, 2025 18:18
Comment thread packages/puppeteer-core/src/cdp/Connection.ts
Comment thread packages/puppeteer-core/src/cdp/Target.ts Outdated
Comment thread packages/puppeteer-core/src/cdp/Target.ts Outdated

@Lightning00Blade Lightning00Blade left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Over all looks really good, thanks for the PR.
Left some comments, that need to be fixed before merging

@alexey-pelykh
alexey-pelykh force-pushed the imp/cdp-FrameManager-typing branch 2 times, most recently from 23bddda to acca085 Compare February 19, 2025 10:15
@alexey-pelykh
alexey-pelykh force-pushed the imp/cdp-FrameManager-typing branch from acca085 to 760c8cd Compare February 19, 2025 10:16
Comment thread packages/puppeteer-core/src/cdp/Connection.ts
@Lightning00Blade

Copy link
Copy Markdown
Collaborator

Thanks for the PR.

@Lightning00Blade
Lightning00Blade merged commit f42428a into puppeteer:main Feb 19, 2025
@alexey-pelykh
alexey-pelykh deleted the imp/cdp-FrameManager-typing branch February 19, 2025 10:57
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.

3 participants