feat (cli) Added Brave Browser debugger support

[kapobajza]

Why

I wanted to debug my app using Brave Browser, but I couldn't. So I decided to add support for it.

How

By extending the LaunchBrowser and LaunchBrowserImpl functionalities.

Test Plan

• Wrote some tests.
• Also built the CLI and tried it locally by starting a new metro bundler using my local build, and then starting the debugger from the metro bundler (by pressing "j")

Checklist

• Documentation is up to date to reflect these changes (eg: https://docs.expo.dev and README.md).
• Conforms with the Documentation Writing Style Guide
• This diff will work correctly for npx expo prebuild & EAS Build (eg: updated a module plugin).

[Kudo]

thanks for bringing this great pr. i'm leaving some comments and will need to test brave on windows and linux before merging it.

[Kudo]

could you share the insight why you would like to move the internal createBrowser from LaunchBrowser.ts to JsInspector.ts?

[kapobajza]

I moved createBrowser to JSInspector.ts because createBrowser was only being used in LaunchBrowser.ts. So I could have exported createBrowser from LaunchBrowser.ts or just move it to JSInspector.ts. The former made more sense to me. Nevertheless, if the latter makes more sense I can adjust the changes accordingly.

And I did all of that just to to inject a LaunchBrowserImpl instance into the launchBrowserAsync function and make it testable. Yeah, sure, I could've used jest.mock instead, but I am not fond of jest.mock and I find this to be the safer way. But if using jest.mock suits this project more, I could also do that.

[Kudo]

yes, i think exporting createBrowser makes more sense and also helps LaunchBrowser.ts isolated.

Our test target would then change to createBrowser and we coud also move the logic here into the createBrowser.

expo/packages/@expo/cli/src/start/server/middleware/inspector/LaunchBrowser.ts

Lines 33 to 35 in 95eaac9

	for (const browserType of [LaunchBrowserTypes.CHROME, LaunchBrowserTypes.EDGE]) {
	const isSupported = await browser.isSupportedBrowser(browserType);
	if (isSupported) {

does that sound good to you?

[kapobajza]

I've understood the first part and I'll export the createBrowser function from LaunchBrowser.ts.

But I am not entirely sure what you mean by the second part: to move the browser is supported check to the createBrowser function. You mean you want to check if the browser is supported, before returning the LaunchBrowserImpl instance from createBrowser? Something like:

export async function createBrowserAsync(
  platform: NodeJS.Platform,
  supportedBrowsers: LaunchBrowserTypes[]
) {
  let browserImpl: LaunchBrowserImpl | undefined = undefined;

  if (platform === 'darwin') {
    browserImpl = new LaunchBrowserImplMacOS();
  }
  if (platform === 'win32' || IS_WSL) {
    browserImpl = new LaunchBrowserImplWindows();
  }
  if (platform === 'linux') {
    browserImpl = new LaunchBrowserImplLinux();
  }

  if (browserImpl === undefined) {
    throw new Error('[LaunchBrowser] Unsupported host platform');
  }

  let supportedBrowsersCount = 0;

  for (const browserType of supportedBrowsers) {
    const isSupported = await browserImpl.isSupportedBrowser(browserType);
    if (isSupported) {
      supportedBrowsersCount++;
    }
  }

  if (supportedBrowsersCount === 0) {
    throw new Error(
      `[LaunchBrowser] Unable to find a browser on the host to open the inspector. Supported browsers: ${supportedBrowsers.join(
        ', '
      )}`
    );
  }

  return browserImpl;
}

If this is what you meant, then I will still have to loop over the supportedBrowsers array inside of LaunchBrowser:

expo/packages/@expo/cli/src/start/server/middleware/inspector/LaunchBrowser.ts

Lines 33 to 38 in 95eaac9

	for (const browserType of [LaunchBrowserTypes.CHROME, LaunchBrowserTypes.EDGE]) {
	const isSupported = await browser.isSupportedBrowser(browserType);
	if (isSupported) {
	return browser.launchAsync(browserType, launchArgs);
	}
	}

In order to call browser.launchAsync with the browserType argument.

I have a feeling that I misunderstood you.

[Kudo]

sorry i didn't describe it clearly. let's show the code directly: https://github.com/expo/expo/compare/%40kudo/pr-25109

if there's still any unclear part, please feel free to let me know.

[kapobajza]

It is clear now. Thank you for the clarification and your code.
I rebased to your branch.

[Kudo]

thanks for helping that. i've got a chance to test brave on linux and windows. please help to revise the launch id accordingly.

[Kudo]

it's ready to go. thanks again for the awesome work.