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
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
690f6e9
feat: add --open to deno serve to open server in browser
HasanAlrimawi Sep 1, 2024
fb4d2af
Merge branch 'main' into open-flag-on-serve
HasanAlrimawi Sep 1, 2024
e8de382
Merge branch 'main' into open-flag-on-serve
HasanAlrimawi Sep 3, 2024
38c0d9d
updated formatting
HasanAlrimawi Sep 3, 2024
e1b52c4
Merge branch 'main' into open-flag-on-serve
HasanAlrimawi Nov 3, 2024
1ffac2f
Merge branch 'main' into open-flag-on-serve
HasanAlrimawi Nov 8, 2024
3ab2371
fix test
HasanAlrimawi Nov 8, 2024
a2e8370
remove unneeded test
HasanAlrimawi Nov 8, 2024
fa17baf
changed the port used in the added test
HasanAlrimawi Nov 8, 2024
8e63dea
Merge branch 'main' into open-flag-on-serve
HasanAlrimawi Nov 12, 2024
14ff348
Merge remote-tracking branch 'upstream/main' into open-flag-on-serve
HasanAlrimawi Nov 18, 2024
4f005ca
Merge remote-tracking branch 'upstream/main' into open-flag-on-serve
HasanAlrimawi Nov 19, 2024
5f2862b
remove un-needed tests and logs
HasanAlrimawi Nov 19, 2024
2ca1a7d
Merge branch 'main' into open-flag-on-serve
HasanAlrimawi Nov 19, 2024
bb1a0b8
Merge branch 'main' into open-flag-on-serve
HasanAlrimawi Nov 19, 2024
47c0635
Merge branch 'main' into open-flag-on-serve
HasanAlrimawi Nov 20, 2024
be1b6f6
re-trigger checks
HasanAlrimawi Nov 20, 2024
cc734a8
Merge branch 'open-flag-on-serve' of https://github.com/HasanAlrimawi…
HasanAlrimawi Nov 20, 2024
7dcb560
Merge branch 'main' into open-flag-on-serve
HasanAlrimawi Nov 21, 2024
449f1a7
Merge branch 'main' into open-flag-on-serve
HasanAlrimawi Nov 24, 2024
d787db9
Merge branch 'main' into open-flag-on-serve
HasanAlrimawi Nov 25, 2024
c64de5b
Merge branch 'main' into open-flag-on-serve
HasanAlrimawi Nov 25, 2024
a45fdeb
Merge branch 'main' into open-flag-on-serve
HasanAlrimawi Nov 27, 2024
655a2a0
Merge branch 'main' into open-flag-on-serve
HasanAlrimawi Dec 1, 2024
5361049
Merge branch 'main' into open-flag-on-serve
HasanAlrimawi Dec 11, 2024
69be52c
Merge branch 'main' into open-flag-on-serve
HasanAlrimawi Dec 12, 2024
3ff08ab
Merge branch 'main' into open-flag-on-serve
HasanAlrimawi Dec 16, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion cli/args/flags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,7 @@ pub struct ServeFlags {
pub port: u16,
pub host: String,
pub worker_count: Option<usize>,
pub open_site: bool,
}

impl ServeFlags {
Expand All @@ -354,6 +355,7 @@ impl ServeFlags {
port,
host: host.to_owned(),
worker_count: None,
open_site: false,
}
}
}
Expand Down Expand Up @@ -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.

.arg(
parallel_arg("multiple server workers")
)
Expand Down Expand Up @@ -4971,6 +4973,7 @@ fn serve_parse(
let host = matches
.remove_one::<String>("host")
.unwrap_or_else(|| "0.0.0.0".to_owned());
let open_site = matches.remove_one::<bool>("open_site").unwrap_or(false);

let worker_count = parallel_arg_parse(matches).map(|v| v.get());

Expand Down Expand Up @@ -5017,6 +5020,7 @@ fn serve_parse(
port,
host,
worker_count,
open_site,
});

Ok(())
Expand Down
23 changes: 23 additions & 0 deletions cli/tools/serve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,33 @@ pub async fn serve(
maybe_npm_install(&factory).await?;

let worker_factory = factory.create_cli_main_worker_factory().await?;

if serve_flags.open_site {
let host: String;
if serve_flags.host == "0.0.0.0" || serve_flags.host == "127.0.0.1" {
host = "http://127.0.0.1".to_string();
} else if serve_flags.host == "localhost" {
host = "http://localhost".to_string();
} else {
host = format!("https://{}", serve_flags.host);
}
let port = serve_flags.port;
let browser_tab_open_result = open::that_detached(format!("{host}:{port}"));
if browser_tab_open_result.is_ok() {
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

}

let hmr = serve_flags
.watch
.map(|watch_flags| watch_flags.hmr)
.unwrap_or(false);

do_serve(
worker_factory,
main_module.clone(),
Expand Down
66 changes: 66 additions & 0 deletions tests/integration/serve_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,43 @@ impl ServeClient {

return self.endpoint.borrow().clone().unwrap();
}

// 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.

let mut buffer = self.output_buf.borrow_mut();
let mut temp_buf = [0u8; 64];
let mut child = self.child.borrow_mut();
let stderr = child.stderr.as_mut().unwrap();

let start = std::time::Instant::now();

// Loop to check for "hello" in the output
loop {
if start.elapsed() > Duration::from_secs(5) {
panic!("timed out waiting for the browser tab to be opened.");
}

let read = stderr.read(&mut temp_buf).unwrap();
buffer.extend_from_slice(&temp_buf[..read]);

let target_output =
"browser on the address that the server is running on".as_bytes();
let target_index = buffer
.windows(target_output.len())
.position(|window| window == target_output);
match target_index {
Some(_) => {
break;
}
None =>
// Sleep for a short duration to avoid busy-waiting
{
std::thread::sleep(Duration::from_millis(5))
}
}
}
}
}

#[tokio::test]
Expand Down Expand Up @@ -198,6 +235,35 @@ async fn deno_serve_no_args() {
assert_eq!(body, "deno serve with no args in fetch() works!");
}

#[tokio::test]
async fn deno_serve_open_browser() {
let client = ServeClientBuilder(
util::deno_cmd()
.current_dir(util::testdata_path())
.arg("serve")
.arg("--open")
.arg("-L")
.arg("debug")
.arg("-A")
.arg("--port")
.arg("0")
.arg("./serve/no_args.ts")
.stdout_piped()
.stderr_piped(),
None,
);

let serve_client = client.entry_point("./serve/no_args.ts").build();
serve_client.check_tab_opened();
let result_unwrapped = serve_client.get().send().await;
let res = result_unwrapped.unwrap();
assert_eq!(200, res.status());

let body = res.text().await.unwrap();
assert_eq!(body, "deno serve with no args in fetch() works!");
serve_client.kill();
}

#[tokio::test]
async fn deno_serve_parallel() {
let client = ServeClient::builder()
Expand Down