Skip to content

Fix no inspectable targets issue with Chrome Browser#4696

Merged
cscheid merged 3 commits intomainfrom
fix/windows-chrome
Mar 8, 2023
Merged

Fix no inspectable targets issue with Chrome Browser#4696
cscheid merged 3 commits intomainfrom
fix/windows-chrome

Conversation

@cderv
Copy link
Collaborator

@cderv cderv commented Mar 8, 2023

Fix #4653

Issue is that somehow Chrome browser open in headless mode with no target available at json/ endpoint.

The logic to solve is to create a new tab if no target is already available. This way there will be a new target with websocket url to use.

Another solution would be to default to json/version endpoint (devtools.Version(option) from devtools.js) to get the browser websocket url

But it feels like the former is good enough and better as we prefer page type for the ws url, so I went with that.

This PR will add to the defaultTarget() handler the creation of a new tab using json/new if no targets is to inspect.
I wasn't sure where to put this here or not, we could also move it upper in the stack in _fetchDebuggerURL() directly, instead of only defaultTarget(). It would be more generic.

This PR also fix an issue I found while working on it - if client is created without error, no need to try maxTries (5) times

Hope this is good. Not sure how to add tests for this quite specific fix.

Tested locally and it works for me.

@cderv cderv requested a review from cscheid March 8, 2023 10:49
@cderv cderv added this to the v1.3 milestone Mar 8, 2023
Copy link
Collaborator

@cscheid cscheid left a comment

Choose a reason for hiding this comment

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

with respect to changes on chrome.js: you'll note that this is a port of a library written by someone else. Fantastic that you found the issue and fixed it.

But I wonder if we should also check the upstream for updates, and set up a more formal fork rather than just change this one directly. (We can do that during 1.4)

@cscheid
Copy link
Collaborator

cscheid commented Mar 8, 2023

Not sure how to add tests for this quite specific fix.

This code path should be getting tested during our mermaid rendering tests. Ideally we'd make an isolated test, but I think we're at least covered.

@cscheid cscheid merged commit 851a4c8 into main Mar 8, 2023
@cderv
Copy link
Collaborator Author

cderv commented Mar 8, 2023

with respect to changes on chrome.js: you'll note that this is a port of a library written by someone else. Fantastic that you found the issue and fixed it.

But I wonder if we should also check the upstream for updates, and set up a more formal fork rather than just change this one directly. (We can do that during 1.4)

I did not realize this one file from a library 🤦 it was in core so I thought it was ours but now I see the head of the file and the copyright and author 😅

Do you know the repo for this ? I can PR there maybe

Otherwise in #4704 we can do a git patch approach as I recently add to updateGithubSourceCodeDependency for PdfExport plugin in Reveal

if (patches) await applyGitPatches(patches)

@cderv cderv deleted the fix/windows-chrome branch March 8, 2023 16:23
@cscheid
Copy link
Collaborator

cscheid commented Mar 8, 2023

@cderv The facepalm is mine! I made the port, so I should know. But I don't remember. It shouldn't be hard to find.

@cderv
Copy link
Collaborator Author

cderv commented Mar 8, 2023

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.

ERROR: "No inspectable targets"-error when rendering PDF containing Mermaid diagram

2 participants

Comments