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

#694 log errors when DevTools cannot be started #760

Closed
wants to merge 5 commits into from

Conversation

karpikpl
Copy link
Contributor

@karpikpl karpikpl commented Jun 4, 2024

This fixes #694

I'm not sure if we should add support for unit testing and Dispose to cleanup after DevTools?

@karpikpl karpikpl requested a review from a team as a code owner June 4, 2024 01:04
Copy link
Collaborator

@waldekmastykarz waldekmastykarz left a comment

Choose a reason for hiding this comment

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

Great start! Let's check a couple of things before we continue

dev-proxy-plugins/Inspection/DevToolsPlugin.cs Outdated Show resolved Hide resolved
dev-proxy-plugins/Inspection/DevToolsPlugin.cs Outdated Show resolved Hide resolved
dev-proxy-plugins/Inspection/DevToolsPlugin.cs Outdated Show resolved Hide resolved
catch (NotSupportedException ex)
{
Logger.LogError(ex, "Error starting {name}. Error finding the browser.", Name);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should stop early here

}

// find if the process is already running
var processes = Process.GetProcessesByName(Path.GetFileNameWithoutExtension(processName));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't work on macOS because the actual process name is stored in Process.MainModule.ModuleName, so we need to update this logic to differentiate across OSes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I'll dust off my MPro and check.

Copy link
Collaborator

Choose a reason for hiding this comment

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

All good, I've done a couple of updates already

@waldekmastykarz
Copy link
Collaborator

I've updated the PR and merged it manually. I've done some changes as to how we detect browsers to accommodate different versions of Edge across OSes. Check out the final commit at: 2d583e3.

Thank you so much for working with us on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warn when browser is already running before starting DevTools
2 participants