Skip to content
This repository has been archived by the owner on Nov 24, 2018. It is now read-only.

provide ability to listen to network events being made and received #177

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

joeyvandijk
Copy link
Contributor

@joeyvandijk joeyvandijk commented Aug 6, 2017

To inspect if network requests are correct, you need the ability to listen to requests and its responses. This is fully possible by https://chromedevtools.github.io/devtools-protocol/tot/Network/ so I added the endpoints to be able to do:

.responseReceivedEvent((response) => { console.log (response) })

and inspect the responses received with Chromeless.

I did not added documentation / websocket-endpoints to first get some feedback about this approach, while the steps described using Chromeless will trigger these events.
I think this is the best approach, because you will be able to define listeners when they are needed or just track all network traffic. All headers are available to inspect like partly requested in #87 and #156 .

Could you provide feedback if this is the right way to add it and then I will add documentation, etc.
Later on we can discuss how to add .stats() and .responses() endpoints to provide the data without any manual steps like described above with .responseReceivedEvent().

@adieuadieu adieuadieu added this to the v1.3 milestone Aug 6, 2017
@adieuadieu
Copy link
Collaborator

@joeyvandijk have you seen #96? Seems maybe there's some overlap between what #96 and this PR are trying to accomplish.

@joeyvandijk
Copy link
Contributor Author

@adieuadieu yes there is overlap, but they are 2 different use-cases. But how can I help with #96 or what do you suggest?

# Conflicts:
#	src/chrome/local-runtime.ts
#	src/types.ts
Copy link
Contributor

@joelgriffith joelgriffith left a comment

Choose a reason for hiding this comment

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

Thanks for sending this in! Would you please update the changelog and docs for these new APIs? I also am wondering about naming event-based API's to something like: onRequestWillBeSent versus requestWillBeSentEvent as the on convention seems more ubiquitous.

@joeyvandijk
Copy link
Contributor Author

@joelgriffith have updated, please review if you have time. 😄

Websocket events can be added later, but lets first determine if this is okay and how people use these events (I will definitely) to add websocket events later on.

@raphaeleidus
Copy link

I have been using this branch for a couple days. SUPER helpful.

I am using it to validate that tracking calls are being made on my pages.

I hope this gets merged soon

@raphaeleidus
Copy link

Is there anyway to use this with serverless before it is merged?

@Aralic
Copy link

Aralic commented Aug 10, 2017

@joelgriffith

I tried your branch feature/networkevents, SUPER helpful.

I hope provide receiveResponse method as soon.

thanks~

Copy link
Collaborator

@adieuadieu adieuadieu left a comment

Choose a reason for hiding this comment

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

Hi @joeyvandijk thank you for having a go at this important feature!

My main feedback concerns the naming of these methods. I agree with the onXXX pattern suggested by @joelgriffith, but we should also take the time to make sure that it's immediately clear what each method does from it's name—even to more novice developers. For example, I'm worried that onDataReceived isn't clear enough. When we're working with CDP, we have the benefit of knowing the domain on which we're using the methods—however this context is missing to a Chromeless user. E.g. internally we know the call is CDP.Network.dataReceived() which provides more information than chromeless.onDataReceived()

I'm wondering whether the implementation of onRequestIntercepted is complete, because, as implemented, we don't enable the request-interception with Network.setRequestInterceptionEnabled? Furthermore, we may need to provide the user with a way to call Network.continueInterceptedRequest as the documentation seems to indicate this is require for events on Network.requesIntercepted. Rather than adding another top-level method the user has to call for continuing a request, we might implement it in a more convenient manner. E.g. instead of a user having to do this:

chromeless.onRequestIntercepted(request => {
  ... // do stuff to manipulate request

  chromeless.continueInterceptedRequest({
    interceptionId: request.interceptionId,
    errorReason: undefined | ErrorReason
    ...
  })
})

We allow for something like this:

chromeless.onRequestIntercepted(request => {
  ... // do stuff to manipulate request

  return true | false // e.g Network.continueInterceptedRequest({ errorReason: true=undefined | false=Aborted })

  // and if the user wants to manipulate the request:

  return {
    error: undefined | ErrorReason
    rawResponse: 'foobar',
    url: 'foobar'
  }
}) 

Then, behind the scene, we call Network.continueInterceptedRequest on the users behalf based on the return value of their callback. Would let us handle the propagation of interceptionId for the user.

This idea isn't fully fleshed out in my mind, but sharing it early rather later. One argument against it might be that it creates a bit of an inconsistent experience between onRequestIntercepted and another event which doesn't have any side-effect coming from it's return value. To alleviate this argument, we could pass a next or continue function as a parameter passed to the onRequestIntercepted callback. E.g.:

chromeless.onRequestIntercepted((request, continue) => {
  ... // do stuff to manipulate request

 continue(true | false) //  e.g. Network.continueInterceptedRequest({ errorReason: true=undefined | false=Aborted })

  // and if the user wants to manipulate the request:

 continue({
    error: undefined | ErrorReason
    rawResponse: 'foobar',
    url: 'foobar'
  })
}) 

@joeyvandijk
Copy link
Contributor Author

@adieuadieu eu I fully understand your point, but it is quite hard to keep fulfulling all wishes of all maintainers when all maintainers have different perspectives. Perspectives that differ are fine by me, but focus or goals where you as a team of maintainers for Chromeless want to be, could help structure this kind of PRs. A PR-template would also help, while I am getting the feeling that I this PR needs too much to be discussed and agreed on inside a single PR.

But I have added this PR because I needed this functionality and while busy coding this PR I thought of many use cases and implementations to improve the setup. It is the focus of implementing a basis and perhaps rethink the API when approaching 2.0.0 that would provide quick but useful iterations, right? Chromeless is a proxy for Chrome headless and therefore I wanted to keep the same API-names to find issues faster on the internet. But sure onDataReceived can be difficult to understand when using websockets, http(s) requests, serviceworkers, etc in a single application.

So my proposal would be to rename the functions to:

  • onNetworkRequestWillBeSent()
  • onNetworkDataReceived()
  • onNetworkRequestServedFromCache()
  • onNetworkLoadingFinished()
  • onNetworkLoadingFailed()
  • onNetworkRequestIntercepted()
  • onNetworkResponseReceived()

and I will implement the requested changes to onNetworkRequestIntercepted because it is missing functionality now.

I would also like to propose new PRs (that I or others can start with) to extend this basic functionality with

  • provide a data-layer where all network events will be stored so we can use that for statistics, referencing the correct requests (onNetworkDataReceived only provides an id), etc.
  • provide API endpoints (for example .stats()) for use-cases based on the above provided endpoints
  • provide websocket event endpoints
  • provide serviceworkers event endpoints

With this approach this PR can be merged, people can use it and discuss their use-cases, propose/request new features, where we can adapt the API refactor in 2.0.0. And perhaps before 2.0.0 like with setUserAgent, setViewport, etc.

Is this a way forward?

@raphaeleidus
Copy link

@joeyvandijk I have been unable to get this to work in remote mode. Have you been able to use this on lambda successfully?

@joeyvandijk
Copy link
Contributor Author

joeyvandijk commented Aug 14, 2017

@raphaeleidus I have made another prototype work with serverless-chrome and the Network event listeners, but not yet tried Chromeless. So it should work, but perhaps you found a bug?

@fleeco
Copy link

fleeco commented Aug 29, 2017

Just chiming in that this would be great to have. I understand you guys are working on rewriting everything to puppeteer but in the interim I think this would fix a lot of problems :)

@squidfunk
Copy link

squidfunk commented Nov 28, 2017

Great work! However, for my case, there are some more methods that I need, so I just hacked the following together which works with the latest version of Chromeless:

chromeless.queue.chrome.runtimeClientPromise.then({ client: { Network } }) => {
  Network.requestWillBeSent(req => {
    console.log(req)
  })
  Network.responseReceived(res => {
    Network.getResponseBody(res).then(data => {
      console.log(res, data)
    })
  })
})

Docs can be found here:
https://chromedevtools.github.io/devtools-protocol/tot/Network/

@adieuadieu adieuadieu removed this from the v1.3 milestone Jan 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants