-
Notifications
You must be signed in to change notification settings - Fork 576
provide ability to listen to network events being made and received #177
base: master
Are you sure you want to change the base?
Conversation
@joeyvandijk have you seen #96? Seems maybe there's some overlap between what #96 and this PR are trying to accomplish. |
@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
There was a problem hiding this 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.
…x type definition bug
@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. |
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 |
Is there anyway to use this with serverless before it is merged? |
I tried your branch I hope provide thanks~ |
There was a problem hiding this 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'
})
})
@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 So my proposal would be to rename the functions to:
and I will implement the requested changes to I would also like to propose new PRs (that I or others can start with) to extend this basic functionality with
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 Is this a way forward? |
@joeyvandijk I have been unable to get this to work in remote mode. Have you been able to use this on lambda successfully? |
@raphaeleidus I have made another prototype work with |
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 :) |
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: |
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:
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()
.