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

Fix buggy behaviour in launchDebugger on macOS #770

Closed
wants to merge 2 commits into from
Closed

Fix buggy behaviour in launchDebugger on macOS #770

wants to merge 2 commits into from

Conversation

Taym95
Copy link
Contributor

@Taym95 Taym95 commented Oct 4, 2019

Fix #762

Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

An idea to make it simple

@@ -77,7 +97,7 @@ function launchChrome(url: string) {
}

function launchDebugger(url: string) {
if (!commandExists(getChromeAppName())) {
if (!commandExists(getChromeCommandName())) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we could get rid of commandExists, getChromeCommandName and getChromeAppName if we try/catch the launchChrome(url) and then fall back to launchDefaultBrowser(url) with the info log. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@thymikee That is what I thought could be done here, but, for some reason, when I played with the code, I could not manage the open library to get to error handling code when I passed some garbage command to it. It seemed it just swallowed any errors... Maybe I will give it another try.

Copy link
Member

Choose a reason for hiding this comment

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

Use async/await interface of open, that should help

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean something like that?

try {
    launchChrome(url);
  } catch (error) {
    launchDefaultBrowser(url);
	  logger.info(
      `For a better debugging experience please install Google Chrome from: 	 
       ${chalk.underline.dim('https://www.google.com/chrome/',
      )}`,);
  }

Copy link
Member

Choose a reason for hiding this comment

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

You're missing the key part: await launchChrome and converting launchChrome into an async function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes sure, but we will need getChromeAppName anyways because launchChrome used it, right?

Copy link
Member

Choose a reason for hiding this comment

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

if it uses it then sure, let it stay

Copy link
Contributor

@StasD StasD Oct 4, 2019

Choose a reason for hiding this comment

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

Looking at open's code gave me some clue. You also need to pass wait: true, for it to pass any errors...
So, it becomes like this:

function launchChrome(url) {
  return (0, _open().default)(url, {
    app: [getChromeAppName()],
    wait: true,
  });
}

async function launchDebugger(url) {
  try {
    await launchChrome(url);
  } catch (err) {
    _cliTools().logger.info(`For a better debugging experience please install Google Chrome from: ${_chalk().default.underline.dim('https://www.google.com/chrome/')}`);
    (0, _launchDefaultBrowser.default)(url);
  }
}

Also need to delete commandExists and commandExistsWindowsSync functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Submitted an alternative PR here: #771 :)

@Taym95 Taym95 closed this Oct 4, 2019
Naturalclar added a commit to Naturalclar/cli that referenced this pull request Oct 7, 2019
Naturalclar added a commit to Naturalclar/cli that referenced this pull request Oct 7, 2019
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.

Buggy behaviour in launchDebugger.js on macOS
3 participants