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: add --open to deno serve to open server in browser #25340

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

HasanAlrimawi
Copy link
Contributor

@HasanAlrimawi HasanAlrimawi commented Sep 1, 2024

This PR addresses issue number #25149

Change:
Supported --open flag with deno serve -> (deno serve --open somescript.ts/js).
The action that takes place is openning the browser on the address that the server is running on.

Changes on code:

  • Added the flag --open to the serve flags as "open_site: bool".

Concerns:
The window tab will be opened on deno serve command no matter whether deno serve succeeds or fails.
The added test since it's a serve command then it starts listening for a request and never shuts down, I couldn't figure out how to shut it down same as other serve tests.

@HasanAlrimawi
Copy link
Contributor Author

@marvinhagemeister @dsherret
Could someone please take a look at this PR?
The problem lies with a test to assure that the change took effect, but because of deno serve nature then the test never shuts down/ends without some intervention which causes the CI checks to fail.

Is there a workaround or should I just remove the test?

@HasanAlrimawi
Copy link
Contributor Author

@bartlomieju @marvinhagemeister
Could someone please take a look at this?

@@ -2875,7 +2877,7 @@ Start a server defined in server.ts, watching for changes and running on port 50
.long("host")
.help("The TCP address to serve on, defaulting to 0.0.0.0 (all interfaces)")
.value_parser(serve_host_validator),
)
).arg(Arg::new("open_site").long("open").help("Open the browser on the address that the server is running on.").action(ArgAction::SetTrue))
Copy link
Member

Choose a reason for hiding this comment

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

Did you run tools/format.js before pushing? This seems not formatted

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 I did, and did it again now, still the same.

Comment on lines 60 to 66
log::info!(
"{}: Opened the browser on the address that the server is running on",
crate::colors::green("deno serve")
);
} else {
log::info!("{}: Couldn't open the browser on the address that the server is running on", crate::colors::red("deno serve"));
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if it worth adding these logs here, does Vite or other tools do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked Vite, it doesn't log on success, but logs "e.g. (Permission denied)" on failure.
Deno already shows logs on failure of serve, so you're right. No need for these logs


// Check if a new browser tab has been opened by checking
// stderr includes the target output expected
fn check_tab_opened(&self) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this test makes sense - on CI there will be no browser to open, while locally people will suddenly get some new tabs. I'd suggest to not add it at all.

@HasanAlrimawi
Copy link
Contributor Author

@bartlomieju
Just a kind reminder of this PR..

@HasanAlrimawi
Copy link
Contributor Author

@bartlomieju
Any shareable updates concerning this PR?

@tobias-edwards
Copy link

Bumping this ^ @bartlomieju

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.

3 participants