-
Notifications
You must be signed in to change notification settings - Fork 853
chore: remove chromedriver dependency #4972
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
base: develop
Are you sure you want to change the base?
Conversation
straker
left a comment
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.
What is the motivation of removing chromedriver from the dev workflow? I'm not sure I like the idea of completely removing chromedriver from the dev workflow. Having latest chromedriver in dev was never a problem and makes development work out of the box. This step requires devs to manually and constantly update their chrome and chromdriver versions. I'd rather CI used the binary and dev used installed chrome + chromedriver.
|
The main driving reason, it adds another point of failure in CI that isn't necessary. If the chromedriver binary fails to download from this for whatever reason, it fails the run. When we already install the chromedriver separately. So it's also just wasting resources in CI by double-downloading every time a box comes online. Secondarily, chromedriver isn't a direct dependency to run the software. It's only needed for a portion of the testing. Which, the contribution guide is updated to help developers understand how to do that. Removing this dependency helps the system become more idempotent. Having This is a convenience for developers yes, but it has a cost that no one needs to be paying. Especially in CI where things are downloaded a lot. And caching the downloaded binary from this as well isn't a huge help, since it does change regularly enough and is just wasting cache resources on something that isn't even used in CI. Caching it would alleviate any failures from external network/host issues. But, why apply that effort when we don't need the dependency to begin with? |
6a2f01d to
ddffa90
Compare
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.
Pull request overview
This PR removes the chromedriver npm package dependency and replaces it with a requirement for developers to manually install and configure ChromeDriver via the CHROMEDRIVER_BIN environment variable. The change only affects developers working on the project, not consumers of the package.
- Replaced automatic chromedriver installation with environment variable requirement
- Added validation to throw clear error messages when
CHROMEDRIVER_BINis not set - Updated contributing documentation with instructions for installing ChromeDriver using
@puppeteer/browsers
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test/integration/full/test-webdriver.js | Added validation to check CHROMEDRIVER_BIN environment variable and throw error if not set |
| test/get-webdriver.js | Added validation to check CHROMEDRIVER_BIN environment variable at module load time |
| package.json | Removed chromedriver from devDependencies |
| package-lock.json | Removed chromedriver and all its transitive dependencies from the lock file |
| CONTRIBUTING.md | Added new "Chrome Driver Configuration" section with installation and setup instructions |
| .github/workflows/format.yml | Reordered steps to setup Node.js before installing dependencies to fix Prettier check |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Some tests require the Chrome Driver binary be installed with a matching Chrome version. This can be achieved with the | ||
| `@puppeteer/browsers` command from the npm registry. | ||
|
|
||
| To get the Chrome For Testing binary run: `npx @puppeteer/browsers install chrome`; |
Copilot
AI
Jan 5, 2026
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.
The command has an unnecessary semicolon at the end. Shell commands typically don't need semicolons unless chaining multiple commands. Remove the semicolon for consistency with line 179.
| To get the Chrome For Testing binary run: `npx @puppeteer/browsers install chrome`; | |
| To get the Chrome For Testing binary run: `npx @puppeteer/browsers install chrome` |
| ### Chrome Driver Configuration | ||
|
|
||
| Some tests require the Chrome Driver binary be installed with a matching Chrome version. This can be achieved with the | ||
| `@puppeteer/browsers` command from the npm registry. | ||
|
|
||
| To get the Chrome For Testing binary run: `npx @puppeteer/browsers install chrome`; | ||
|
|
||
| To get the Chrome Driver run: `npx @puppeteer/browsers install chromedriver` |
Copilot
AI
Jan 5, 2026
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.
Inconsistent capitalization. "Chrome Driver" should be "ChromeDriver" to match the standard naming convention used elsewhere in the documentation and in the environment variable name CHROMEDRIVER_BIN.
| ### Chrome Driver Configuration | |
| Some tests require the Chrome Driver binary be installed with a matching Chrome version. This can be achieved with the | |
| `@puppeteer/browsers` command from the npm registry. | |
| To get the Chrome For Testing binary run: `npx @puppeteer/browsers install chrome`; | |
| To get the Chrome Driver run: `npx @puppeteer/browsers install chromedriver` | |
| ### ChromeDriver Configuration | |
| Some tests require the ChromeDriver binary be installed with a matching Chrome version. This can be achieved with the | |
| `@puppeteer/browsers` command from the npm registry. | |
| To get the Chrome For Testing binary run: `npx @puppeteer/browsers install chrome`; | |
| To get the ChromeDriver run: `npx @puppeteer/browsers install chromedriver` |
| ### Chrome Driver Configuration | ||
|
|
||
| Some tests require the Chrome Driver binary be installed with a matching Chrome version. This can be achieved with the | ||
| `@puppeteer/browsers` command from the npm registry. | ||
|
|
||
| To get the Chrome For Testing binary run: `npx @puppeteer/browsers install chrome`; | ||
|
|
||
| To get the Chrome Driver run: `npx @puppeteer/browsers install chromedriver` |
Copilot
AI
Jan 5, 2026
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.
Inconsistent capitalization. "Chrome Driver" should be "ChromeDriver" to match the standard naming convention used elsewhere in the documentation and in the environment variable name CHROMEDRIVER_BIN.
| ### Chrome Driver Configuration | |
| Some tests require the Chrome Driver binary be installed with a matching Chrome version. This can be achieved with the | |
| `@puppeteer/browsers` command from the npm registry. | |
| To get the Chrome For Testing binary run: `npx @puppeteer/browsers install chrome`; | |
| To get the Chrome Driver run: `npx @puppeteer/browsers install chromedriver` | |
| ### ChromeDriver Configuration | |
| Some tests require the ChromeDriver binary be installed with a matching Chrome version. This can be achieved with the | |
| `@puppeteer/browsers` command from the npm registry. | |
| To get the Chrome For Testing binary run: `npx @puppeteer/browsers install chrome`; | |
| To get the ChromeDriver run: `npx @puppeteer/browsers install chromedriver` |
| ### Chrome Driver Configuration | ||
|
|
||
| Some tests require the Chrome Driver binary be installed with a matching Chrome version. This can be achieved with the | ||
| `@puppeteer/browsers` command from the npm registry. | ||
|
|
||
| To get the Chrome For Testing binary run: `npx @puppeteer/browsers install chrome`; | ||
|
|
||
| To get the Chrome Driver run: `npx @puppeteer/browsers install chromedriver` |
Copilot
AI
Jan 5, 2026
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.
Inconsistent capitalization. "Chrome Driver" should be "ChromeDriver" to match the standard naming convention used elsewhere in the documentation and in the environment variable name CHROMEDRIVER_BIN.
| ### Chrome Driver Configuration | |
| Some tests require the Chrome Driver binary be installed with a matching Chrome version. This can be achieved with the | |
| `@puppeteer/browsers` command from the npm registry. | |
| To get the Chrome For Testing binary run: `npx @puppeteer/browsers install chrome`; | |
| To get the Chrome Driver run: `npx @puppeteer/browsers install chromedriver` | |
| ### ChromeDriver Configuration | |
| Some tests require the ChromeDriver binary be installed with a matching Chrome version. This can be achieved with the | |
| `@puppeteer/browsers` command from the npm registry. | |
| To get the Chrome For Testing binary run: `npx @puppeteer/browsers install chrome`; | |
| To get the ChromeDriver run: `npx @puppeteer/browsers install chromedriver` |
| To get the Chrome Driver run: `npx @puppeteer/browsers install chromedriver` | ||
|
|
||
| > [!NOTE] | ||
| > Do check the commands arguments, you can set a path to install to so they |
Copilot
AI
Jan 5, 2026
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.
Grammatical issue. "command's arguments" should be "command's arguments" or better yet "the command's arguments" for clarity.
| > Do check the commands arguments, you can set a path to install to so they | |
| > Do check the command's arguments, you can set a path to install to so they |
| if (!chromedriverPath || chromedriverPath.trim() === '') { | ||
| throw new Error( | ||
| 'CHROMEDRIVER_BIN environment variable is not set. Please set it to the path of the chromedriver executable.' | ||
| ); | ||
| } |
Copilot
AI
Jan 5, 2026
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.
The validation logic for chromedriverPath is duplicated between this file and test/get-webdriver.js. Consider extracting this validation into a shared utility function to maintain consistency and reduce duplication.
| ### Chrome Driver Configuration | ||
|
|
||
| Some tests require the Chrome Driver binary be installed with a matching Chrome version. This can be achieved with the | ||
| `@puppeteer/browsers` command from the npm registry. |
Copilot
AI
Jan 5, 2026
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.
Missing period at the end of the sentence. It should be "...from the npm registry." to maintain consistency with the rest of the documentation.
|
As a one-time contributor to axe-core, the pr is mostly fine (see point 4 below). However from a maintainer standpoint I'm not sure I like this approach. All bug fixes and testing happen by running With that in mind, here are the things that would have to be solved in order to accept the changes proposed:
|
Installing binaries without explicit user consent is an anti-pattern in package management. These types of automated installations can introduce significant security risks. In fact, similar practices contributed to the npm supply chain attacks we saw last year, which ultimately led to npm deprecating legacy authentication tokens in favor of more secure OIDC-based authentication. Instead, I'd propose we focus on improving the developer experience through:
This approach respects the principle of least surprise and gives users control over what gets installed in their environment, while still making the setup process straightforward. Happy to discuss alternative approaches that balance convenience with security best practices. However, the approach to make this all smoother should be detached from removing the problematic dependency that we have. As to require a full and complete implementation to maintain the ease-of-use in one go means the change set easily becomes a thousand line delta pull request.
Valid point on the docs being unclear on where to store things. I'll get that clarified and updated for sure. It might be we introduce a |
This patch removes the dependency on
chromedriverentirely from the package. Now we check for the environment variable and if it isn't present we fail with a meaningful error message. Since this is only used in tests, it only impacts developers working in the project and not consumers of it.The contributing documentation has been updated to include information on how to get the needed binaries installed and configured.
Upon opening this PR, the Prettier check was failing. The reason for this was the dependencies were installed before node was setup on the version we run on locally. So Prettier was then expecting a newer node version then what it should be operating on. To fix this, I swapped the order of node-setup and
Install Dependenciesin the formatter workflow.No QA Required