-
-
Notifications
You must be signed in to change notification settings - Fork 184
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
Add support for downloading the chromedriver after installation #332
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @niehusst! Thanks for the PR, it is much easier to review now.
I think we can improve it a bit. I think there is a bit of feature envy going on in the chromedriver.js file, it is using a lot of things from install.js.
I'd prefer that the installation functions be moved into chromedriver.js, and install.js would then use those functions, and continued to be a install only script, not used in the module itself.
This would also eliminate a bit of code duplication that is showing up at the current iteration.
Good idea @giggio. Hopefully this little refactor cleans things up a bit without making the PR too convoluted. |
The CI build has failed, but the error logs aren't the most helpful, so I'm not really sure what the problem is. Some of the failures look like they're git permissions related, and others seem to have stalled during what looks like environment setup? Lmk if you can make any more sense of the errors. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@giggio did you ever get a chance to look at this again? |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This is the same code changes as #278 (all substantial code changes are thanks to @r3pwn) but without any of the refactoring that made it seem like a bigger PR than it actually was. Hopefully, this will be easier to review.
Using the
.download
async function, we can now do things likebefore any tests get run.
Additionally, some options are supported.
We can pass in
download_version
orcdn_url
like such:download_version is able to be passed in as the full version of chrome ("84.0.4147.105") or just the major version ("84")
I believe this is the functionality requested by @HaimBendanan in issue #256