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

feat: make docusaurus serve automatically open in browser #7500

Merged
merged 5 commits into from
May 27, 2022
Merged

feat: make docusaurus serve automatically open in browser #7500

merged 5 commits into from
May 27, 2022

Conversation

Zamiell
Copy link
Contributor

@Zamiell Zamiell commented May 26, 2022

self-explanatory

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label May 26, 2022
@netlify
Copy link

netlify bot commented May 26, 2022

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 9d9c043
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/628f79a6fe53d9000846d7c0
😎 Deploy Preview https://deploy-preview-7500--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented May 26, 2022

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟢 90 🟢 100 🟢 100 🟢 100 🟢 90 Report
/docs/installation 🟠 85 🟢 99 🟢 100 🟢 100 🟢 90 Report

@Josh-Cena Josh-Cena added the pr: new feature This PR adds a new API or behavior. label May 26, 2022
@Josh-Cena Josh-Cena changed the title feat: add automatic url open to npm run serve feat: add automatic open-in-browser to docusaurus serve May 26, 2022
Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

Works very well. Thanks!

This could technically be breaking, as I'm not sure what the behavior of this would be in the CI environment. However, we do warn users to not use serve for actual deployment. I'll leave the choice to @slorber. The general direction is definitely desired: I've thought about the same.

@Zamiell
Copy link
Contributor Author

Zamiell commented May 26, 2022

This could technically be breaking, as I'm not sure what the behavior of this would be in the CI environment.

See: https://www.scivision.dev/ci-detect-environment-variable/

Maybe the if statement should be changed from:

if (cliOptions.open) {

to:

// This function lives in utils.ts or whatever
function inCI() {
  return process.env["CI"].toLower() === "true"; 
}
if (cliOptions.open && !inCI) {

@Josh-Cena
Copy link
Collaborator

We already have process.env.CI checks in deploy, and it's relatively lightweight and stable so I don't think a util function is necessary. But adding this check certainly seems desirable.

@Zamiell
Copy link
Contributor Author

Zamiell commented May 26, 2022

Well the check should also be added to npm start as well, right? Thus, the util function.

@Josh-Cena
Copy link
Collaborator

start is never meant to be run in deployment. People ought to be burnt if they do 🔥

@Josh-Cena Josh-Cena changed the title feat: add automatic open-in-browser to docusaurus serve feat: make docusaurus serve automatically open in browser May 26, 2022
@Zamiell
Copy link
Contributor Author

Zamiell commented May 26, 2022

Still, it feels less error prone to deal with both at once.
I think the actual best solution is to remove both openBrowser calls with a call to a util function called something like tryOpenBrowser with an early return for CI.
That way if a 3rd openBrowser call is ever added somewhere else, it is less likely to have this bug, since the programmer would presumably copy from the existing usages.

@Josh-Cena
Copy link
Collaborator

FWIW, I don't particularly like lightweight encapsulations—even less so when it's used to guard against a situation that never exists. docusaurus start starts a dev server with watch and hot reload, which doesn't make much sense in the CI/deployment environment. Had we started with the --no-open option, I won't even add the check to serve. For now, I think simply changing it to if (!process.env.CI && cliOptions.open) would suffice.

@slorber
Copy link
Collaborator

slorber commented May 27, 2022

👍

@slorber slorber merged commit 35e6351 into facebook:main May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants