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

Added fn to get latest chrome revision(requires fetch feature) #374

Closed

Conversation

initprism
Copy link
Contributor

No description provided.

@mdrokz mdrokz added the enhancement New feature or request label Feb 16, 2023
@mdrokz
Copy link
Collaborator

mdrokz commented Feb 16, 2023

Hey @initprism thanks for the PR but can you explain the use of this method to fetch latest chrome revision?

@initprism
Copy link
Contributor Author

The latest_chrome_revision function returns the latest chrome revision in the platform environment.
This can be used by modifying the revision of FecherOptions if you want to use the latest Chrome version.

let revision =  headless_chrome::browser::latest_chrome_revision()?;
let fetcher_options = headless_chrome::FetcherOptions::default()
    .with_revision(revision);

let launch_options = headless_chrome::LaunchOptionsBuilder::default()
    .fetcher_options(fetcher_options)
    .build()?;
let browser = Browser::new(launch_options)?;

IMO, I don't think it's correct to always install the latest chrome revision when enable the fetch feature.
Also, the reason this function is not included in the fetcher module is that the lib does not export fetcher to pub. This will have to be reflected later to be moved to the fetcher module or written in the appropriate method.

@masc-it
Copy link
Contributor

masc-it commented Feb 18, 2023

I think this is really useful @initprism, maybe it can be re-designed in a way that extends the current .with_revision("some-version") function, rather than adding a new function, e.g. we can define a Version enum that can be passed to it:

enum Version {
    Specific(S),  // S: Into<String>
    Latest
}

.with_revision(Version::Specific("634997"))
or 
.with_revision(Version::Latest)

@mdrokz
Copy link
Collaborator

mdrokz commented Feb 20, 2023

The latest_chrome_revision function returns the latest chrome revision in the platform environment. This can be used by modifying the revision of FecherOptions if you want to use the latest Chrome version.

let revision =  headless_chrome::browser::latest_chrome_revision()?;
let fetcher_options = headless_chrome::FetcherOptions::default()
    .with_revision(revision);

let launch_options = headless_chrome::LaunchOptionsBuilder::default()
    .fetcher_options(fetcher_options)
    .build()?;
let browser = Browser::new(launch_options)?;

IMO, I don't think it's correct to always install the latest chrome revision when enable the fetch feature. Also, the reason this function is not included in the fetcher module is that the lib does not export fetcher to pub. This will have to be reflected later to be moved to the fetcher module or written in the appropriate method.

Makes sense thanks for the explanation !

Comment on lines 502 to 516
if cfg!(target_os = "linux") {
url = format!("{}/Linux_x64/LAST_CHANGE", url);
} else if cfg!(all(target_os = "macos", target_arch = "aarch64")) {
url = format!("{}/Mac_Arm/LAST_CHANGE", url);
} else if cfg!(all(target_os = "macos", not(target_arch = "aarch64"))) {
url = format!("{}/Mac/LAST_CHANGE", url);
} else if cfg!(windows) {
url = format!("{}/Win_x64/LAST_CHANGE", url);
} else {
// This code will panic on unsupported platforms.
#[allow(unreachable_code)]
{
unimplemented!("This platform is not supported")
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe it would be better to refactor this into a match statement ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactoring to match statement looks good. And reflecting the definition suggested by @masc-it looks good.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Refactoring to match statement looks good. And reflecting the definition suggested by @masc-it looks good.

Cool i will wait for you to add these changes. Then we can merge this PR

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'm sorry I closed this PR by mistake. (;_;)
please check #380

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants