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: add sendRaw function #441

Closed
wants to merge 1 commit into from
Closed

Conversation

flotwig
Copy link

@flotwig flotwig commented Dec 29, 2020

Closes #439

Adds a sendRaw function that lets the user manually construct a CDP message. This way, the user has control over parameters like sessionId, unlocking the full capabilities of CDP.

@Niek
Copy link

Niek commented Jan 13, 2021

Nice addition, AFAIK this is necessary to support commands like Target.attachToBrowserTarget()

@cyrus-and
Copy link
Owner

AFAIK this is necessary to support commands like Target.attachToBrowserTarget()

@Niek what's the use of this method since is only available from the browser session itself?

@flotwig
Copy link
Author

flotwig commented Mar 9, 2021

what's the use of this method since is only available from the browser session itself?

Now chrome-remote-interface can be used when connected directly to the browser, not to any specific target. In stdio mode, this is the only option available, since there's no way to list targets or signal that you want to connect to a specific target besides via CDP commands like Target.attachToBrowserTarget.

@cyrus-and
Copy link
Owner

@flotwig hmm in stdio mode you're already in the "browser target", as you can perform operations like Target.createBrowserContext that are not usually allowed in regular (e.g., tabs) targets. From there you can operate on targets, list, create, attach and do whatever you would do in the browser/global target, i.e., the one otherwise reachable via the webSocketDebuggerUrl WebSocket field in the /json/version endpoint. So Target.attachToBrowserTarget seems redundant.

@flotwig
Copy link
Author

flotwig commented Mar 10, 2021

Ah yeah, we should be talking about Target.attachToTarget, not Target.attachToBrowserTarget here. 🤦

Is there another way to attach to a per-tab target from the browser target besides calling Target.attachToTarget? After using Target.attachToTarget, the sessionId that is yielded from that command must be sent with every command for the target, which was the motivation for this PR.

@cyrus-and
Copy link
Owner

No OK, my question was (for @Niek) about Target.attachToBrowserTarget.

Is there another way to attach to a per-tab target from the browser target besides calling Target.attachToTarget? After using Target.attachToTarget, the sessionId that is yielded from that command must be sent with every command for the target, which was the motivation for this PR.

@flotwig the alternative is the deprecated Target.sendMessageToTarget, so yeah this PR makes sense.


BTW, I can't seem to make it work, Chrome simply doesn't reply anything to messages that include a sessionId parameter:

$ wscat -c ws://127.0.0.1:9222/devtools/browser/322f0530-a2ea-4430-aa1e-70f897a30e4c
> {"id":0,"method":"Target.getTargets","params":{}}
< {"id":0,"result":{"targetInfos":[{"targetId":"153B060BA08BB627A6205846A1799A46","type":"page","title":"about:blank","url":"about:blank","attached":false,"canAccessOpener":false,"browserContextId":"E26C41C6E1F55135ABDDCEBB1BBB071E"}]}}
> {"id":0,"method":"Target.attachToTarget","params":{"targetId":"153B060BA08BB627A6205846A1799A46"}}
< {"method":"Target.attachedToTarget","params":{"sessionId":"AB8DD70A146AD1EEC8363C92E4D5DEEE","targetInfo":{"targetId":"153B060BA08BB627A6205846A1799A46","type":"page","title":"about:blank","url":"about:blank","attached":true,"canAccessOpener":false,"browserContextId":"E26C41C6E1F55135ABDDCEBB1BBB071E"},"waitingForDebugger":false}}
< {"id":0,"result":{"sessionId":"AB8DD70A146AD1EEC8363C92E4D5DEEE"}}
> {"id":0,"method":"Page.navigate","sessionId":"AB8DD70A146AD1EEC8363C92E4D5DEEE","params":{"url":"http://example.com"}}
> # no reply...

Where ws://127.0.0.1:9222/devtools/browser/322f0530-a2ea-4430-aa1e-70f897a30e4c is the browser WebSocket:

$ curl http://localhost:9222/json/version
{
   "Browser": "HeadlessChrome/91.0.4441.0",
   "Protocol-Version": "1.3",
   "User-Agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) HeadlessChrome/91.0.4441.0 Safari/537.36",
   "V8-Version": "9.1.60",
   "WebKit-Version": "537.36 (@151c9b31e30d132a7a859f41f49bd44d7b008675)",
   "webSocketDebuggerUrl": "ws://localhost:9222/devtools/browser/322f0530-a2ea-4430-aa1e-70f897a30e4c"
}

@cyrus-and
Copy link
Owner

Chrome simply doesn't reply anything to messages that include a sessionId parameter

With the only exception, funnily enough, of the sessionId returned by the Target.attachToBrowserTarget method, which is useless IMHO.

cyrus-and added a commit that referenced this pull request Mar 13, 2021
This closes #439 (feature request) and closes #441 as it provides a
more convenient implementation.
@cyrus-and
Copy link
Owner

In the meanwhile I pushed an alternate implementation in a separate branch that does not require an additional function, now the sessionId is just another optional argument to send() and all the other variants, for example:

await Page.navigate(sessionId, {url: 'https://github.com'});

@Niek
Copy link

Niek commented Mar 15, 2021

@cyrus-and Sorry, Target.attachToBrowserTarget was a bad example.

I tested the new branch and it works great:

 ./bin/client.js inspect
>>> Target.getTargets()
{
  targetInfos: [
    {
      targetId: 'C104A7A2F6C6B29840C1EAFD9E7F4A22',
      type: 'page',
      title: 'about:blank',
      url: 'about:blank',
      attached: true,
      canAccessOpener: false,
      browserContextId: 'C689EBAB4EF2567951424D1C7E0794FA'
    }
  ]
}
>>> Target.attachToTarget({targetId: 'C104A7A2F6C6B29840C1EAFD9E7F4A22'})
{ sessionId: '5B8328334DD51F8EFDBD17C941C42C58' }
>>> Page.navigate({sessionId: '5B8328334DD51F8EFDBD17C941C42C58', url: 'https://github.com'});
{
  frameId: 'C104A7A2F6C6B29840C1EAFD9E7F4A22',
  loaderId: '78B81222D9D6211F918787268E249B9B'
}

Looking forward to see this released!

@cyrus-and
Copy link
Owner

@Niek uhm, I'm afraid it doesn't work that way, you're just passing sessionId as another parameter and it gets ignored as it is not a parameter of Page.navigate. That example would also work with master version.

In the new branch the correct syntax is:

Page.navigate('5B8328334DD51F8EFDBD17C941C42C58', { url: 'https://github.com'})

But it hangs, as I pointed out in this comment (where I use the browser target, which I think is the only context where the sessionId can be used AFAIK).

I'm not aware of your use case but it looks like you don't really need to use sessionId. But if I'm wrong I'd love to see a minimal examples that successfully manages to use it, even with raw WebSocket messages.

@Niek
Copy link

Niek commented Mar 15, 2021

@cyrus-and Oops, I feel silly. This is the way to get it working (you need to flatten the session):

>>> Target.getTargets()
{
  targetInfos: [
    {
      targetId: 'AE5249680D5E2E5FE1AEBC1FE029EC8F',
      type: 'page',
      title: 'about:blank',
      url: 'about:blank',
      attached: true,
      canAccessOpener: false,
      browserContextId: '1FB2FCD4E41A007E3938DED4678A5933'
    }
  ]
}
>>> Target.attachToTarget({targetId: 'AE5249680D5E2E5FE1AEBC1FE029EC8F', flatten: true})
{ sessionId: '8C45BC912828F26DD634268FF23A1570' }
>>> Page.navigate('8C45BC912828F26DD634268FF23A1570', {url: 'https://github.com'});
{
  frameId: 'AE5249680D5E2E5FE1AEBC1FE029EC8F',
  loaderId: '2154A11FF6AE82267E2BE7ABA9C7D7B7'
}

@Niek
Copy link

Niek commented Mar 15, 2021

I'm not aware of your use case but it looks like you don't really need to use sessionId. But if I'm wrong I'd love to see a minimal examples that successfully manages to use it, even with raw WebSocket messages.

My 2c on this: for simply navigating a page the sessionId is useless, however there are other cases where it's really useful. See for example this bugreport: https://bugs.chromium.org/p/chromium/issues/detail?id=1070568

Without sessionId it's impossible to attach to all targets in time. In addition, the Chromium developers are moving towards supporting sessionId only. So this is a very important feature to have IMHO.

@cyrus-and
Copy link
Owner

@Niek

This is the way to get it working (you need to flatten the session):

LOL now I feel silly, I thought it was enabled by default...

My 2c on this: for simply navigating a page the sessionId is useless, however there are other cases where it's really useful. See for example this bugreport: https://bugs.chromium.org/p/chromium/issues/detail?id=1070568

That makes perfectly sense, thank you.

So this is a very important feature to have IMHO.

Yup, let me clean up a few things and I'll merge it in a few hours.

cyrus-and added a commit that referenced this pull request Mar 15, 2021
Allow to filter events by `sessionId`. There is one gotcha that when
using promises without filter, the `sessionId` is not returned due to
the limitation of promises to return only one value, and the fact that
changing the return type would be a breaking change.

Related to #441.
@cyrus-and
Copy link
Owner

There is one problem though, events should support sessionId too so that it is possible to listen only for those belonging to one particular session.

The event filtering part should be fairly easy to implement, the only problem raises when events are registered via a promise and without any sessionId in that cases (the present case actually) there should be a way to tell from what session an event is coming from. Just adding a parameter will work for callbacks, but promises can only return one value, and changing the API (to return {params, sessionId} instead of just params) would be a breaking change...

@cyrus-and
Copy link
Owner

I pushed a possible implementation in that branch.

cyrus-and added a commit that referenced this pull request Mar 15, 2021
Allow to filter events by `sessionId`. There is one gotcha that when
using promises without filter, the `sessionId` is not returned due to
the limitation of promises to return only one value, and the fact that
changing the return type would be a breaking change.

Related to #441.
cyrus-and added a commit that referenced this pull request Mar 15, 2021
Allow to filter events by `sessionId`. There is one gotcha that when
using promises without filter, the `sessionId` is not returned due to
the limitation of promises to return only one value, and the fact that
changing the return type would be a breaking change.

Related to #441.
cyrus-and added a commit that referenced this pull request Mar 15, 2021
Allow to filter events by `sessionId`. There is one gotcha that when
using promises without filter, the `sessionId` is not returned due to
the limitation of promises to return only one value, and the fact that
changing the return type would be a breaking change.

Related to #441.
@cyrus-and cyrus-and closed this in 082ac9d Mar 22, 2021
cyrus-and added a commit that referenced this pull request Mar 22, 2021
@flotwig
Copy link
Author

flotwig commented Mar 23, 2021

@cyrus-and looks great, thanks!

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.

sessionId support
3 participants