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

CLI: Fix open storybook in default browser #16844

Conversation

richorrichard
Copy link
Contributor

Issue: #15316

What I did

Updated open-in-browser.ts to find the user's default browser, and attempt to honor that default browser (when default is Firefox or Safari) instead of automatically opening in a Chromium-based browser.

  • Included 2 new npm packages:
    • x-default-browser to detect the user's system default browser
    • open to handle cross-platform versions of Firefox

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Nov 30, 2021

Nx Cloud Report

CI ran the following commands for commit 3a7ab52. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

@richorrichard Thanks so much for taking this on! 🙏 A couple comments/questions:

name: 'firefox',
},
});
} catch (error) {
Copy link
Member

Choose a reason for hiding this comment

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

can we move the try/catch around the whole thing to remove duplicate code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call - I adjusted in the following commit.

browser by default.
`);
}
} else {
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't the logic be switched here where we use betterOpn for chrome/chromium only, and then use open for everything else? my understanding of the original issue is that betterOpn always prefers chrome if it's available and that's what was annoying users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is also much simpler. I was originally turned off to this idea because of the way betterOpn triages versions of chrome. I updated to test for Chrome and checked it against every browser I could get my hands on for Mac and this approach works wonderfully, and it's much simpler to read. Thanks for the feedback and review!

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Awesome -- thanks so much for the follow-up, this turned out great!

@shilman shilman changed the title FIX #15316 open storybook in default browser CLI: Fix open storybook in default browser Dec 1, 2021
@richorrichard
Copy link
Contributor Author

You're very welcome! I'm looking forward to moving on to my next contribution :)

Last question - I see there are non-required failed checks. Is that something I should dig into and try to fix? Let me know if there's anything else I need to do to prepare it for merge.

@yannbf
Copy link
Member

yannbf commented Dec 2, 2021

Hey @richorrichard nice contribution! I originally swapped opn with better-opn because it reuses browser tabs rather than always opening a new one (if you kill storybook and run the command again). Do you know if that is still the case? If so, even though the browser will be better respected, it will still be quite annoying.

Edit: just tried your branch and indeed the behavior of opn is the same as before, it will keep opening new tabs. For me, that is a regression from the current behavior. I stand corrected. After checking the PR in more details I noticed that the previous functionality is kept for chromium based browsers. Still, would it be possible to make a PR to better-opn where the default browser is taken into account? That would be the best of both worlds!

@shilman
Copy link
Member

shilman commented Dec 2, 2021

@yannbf doesn't it reuse tabs if your browser is set the chrome? and respect your setting if your browser is not set to chrome? i haven't tested the branch but assuming that's the behavior i would consider that a fix not a regression. i agree having the fix in better-opn would be even better than the current solution

@richorrichard
Copy link
Contributor Author

@yannbf and @shilman I agree that using better-opn by itself would be an ideal end state, but it would take more work than I think is reasonable for a small issue like this, and I believe the better-opn strategy would be more fragile than using it only for the chrome case.

My reasoning is that open handles the logic of opening any given browser cross-platform better than better-opn. better-opn CAN open other explicitly desired browsers, but it requires passing in that browser's binary as an environment variable, and that variable is platform-specific. So, I believe using open is less fragile to use as our base case gracefully handle non-chrome browsers, and use better-opn to handle chrome-based browsers because it does a better job of targeting the active window/tab in Chrome.

@shilman
Copy link
Member

shilman commented Dec 2, 2021

Well said @richorrichard ... works for me! 🙏

@shilman shilman merged commit 9ec5bc4 into storybookjs:next Dec 2, 2021
@richorrichard richorrichard deleted the 15316-open-storybook-in-default-browser branch December 6, 2021 16:41
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.

None yet

3 participants