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(worker): Allow Workers to emit their network events #2717

Closed
wants to merge 5 commits into from

Conversation

chad3814
Copy link

small patch that hooks Workers into the NetworkManager so that they can emit network events

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@chad3814
Copy link
Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@chad3814 chad3814 changed the title Allow Workers to emit their network events feature(Worker): Allow Workers to emit their network events Jun 11, 2018
@chad3814 chad3814 changed the title feature(Worker): Allow Workers to emit their network events feat(worker): Allow Workers to emit their network events Jun 11, 2018
@JoelEinbinder
Copy link
Collaborator

Thanks for the PR!

Maybe we should have these events fired on the page, like we do for frames. Wdyt @aslushnikov ?

Also needs a test.

@chad3814
Copy link
Author

yeah I was sure how to make some tests, I'm not familiar with how the coverage tests work, there don't appear to be specific tests for the Page versions, they just look like they get exercised in the process of other tests. Something similar could be done in Worker but where do those tests get their worker script? I can change it to open another remote resource.

@chad3814
Copy link
Author

Okay wrote tests :)

@chad3814
Copy link
Author

The travis build had a timeout on a different test I didn't touch and AppVeyor succeeded on that test...

@aslushnikov
Copy link
Contributor

@chad3814 we played with this and found critical issues with nested targets in DevTools protocol:

  • events are not fired when request interception is enabled in the main target
  • network throttling doesn't quite work with subtargets

We'll have to fix these in Chromium as a part of #2548. There's no ETA for OOPIFs since it's unclear what else is broken in puppeteer with multiple subtargets.

I'll close this PR for now since there's no way we can move forward here without upstream being fixed.
I filed #2781 to track the feature.

Thank you for the PR!

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

4 participants