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

Progress bars for "Downloading" messages #1320

Closed
ry opened this issue Dec 12, 2018 · 7 comments · Fixed by #15814
Closed

Progress bars for "Downloading" messages #1320

ry opened this issue Dec 12, 2018 · 7 comments · Fixed by #15814
Assignees

Comments

@ry
Copy link
Member

ry commented Dec 12, 2018

Should be completely in Rust.
This looks like a good library to use: https://github.com/mitsuhiko/indicatif

Starting points is here:

eprintln!("Downloading {}", module_name);

(After #975 the "Compiling" messages can also be added to the progress bar. It will move the compilation step into Rust. Currently "Compiling" is printed from JS.)

@manyuanrong
Copy link
Contributor

@ry I've been watching this project for several months and I really want to be involved. Can I try this issue?

@ry
Copy link
Member Author

ry commented Apr 16, 2019

Async module loading has now landed. It should be quite approachable now to do proper progress bars.

@bartlomieju
Copy link
Member

Waiting for denoland/deno_third_party#32 to resolve this

@ry
Copy link
Member Author

ry commented Apr 27, 2019

Ref #2229

@bartlomieju
Copy link
Member

@ry moving discussion here to keep it in one place


@bartlomieju I'd rather there be a "ProgressBar" object (which should wrap whichever progress bar crate we're using) which can be added to ThreadSafeState. Then you'd have something like ProgressBar::start(action: String) and ProgressBar::end(action: String) ? Not sure about that exact API, but some functionality like that.

Originally posted by @ry in #2057 (comment)



@bartlomieju I like how ninja does its progress - which is very simple - it looks like

[4/34] STAMP obj/cli/msg_rs.stamp

maybe we can just do that manually in a few lines of code.

Originally posted by @ry in #2229 (comment)


Taking both comments into consideration:

  • When deno starts we don't know which files are cached so we don't know how many files will be downloaded. That means we need to store and update number of files to download and already downloaded as new imports are being "discovered".

Then when file is being downloaded around here:

deno/cli/deno_dir.rs

Lines 560 to 570 in a4551c8

filename: &str,
) -> impl Future<Item = Option<ModuleMetaData>, Error = DenoError> {
use crate::http_util::FetchOnceResult;
{
eprintln!("Downloading {}", module_name);
}
let filename = filename.to_owned();
let module_name = module_name.to_owned();
// We write a special ".headers.json" file into the `.deno/deps` directory along side the

number of files to download should be incremented, and message displayed:

[{no_finished_downloads}/{no_downloads}] DOWNLOAD {filename}
[0/1] DOWNLOAD https://deno.land/std/prettier/main.ts
[0/4] DOWNLOAD https://deno.land/std/prettier/prettier.ts
[1/4] DOWNLOAD https://deno.land/std/prettier/prettier.ts
...

Lastly around here:

deno/cli/deno_dir.rs

Lines 648 to 650 in a4551c8

}
Ok(Loop::Break(Some(ModuleMetaData {
module_name: module_name.to_string(),

number of files downloaded should be incremented.

I guess it makes most sense to store those numbers in Metrics. The same thing applies for compiled files.

@bartlomieju
Copy link
Member

Done in #2309

@ry ry closed this as completed May 20, 2019
@ry
Copy link
Member Author

ry commented Aug 31, 2022

We reverted this - but this is still a desirable feature. I hope it can be done minimally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants