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

Optional peer dep installation #3215

Closed
acolytec3 opened this issue Jan 6, 2024 · 2 comments
Closed

Optional peer dep installation #3215

acolytec3 opened this issue Jan 6, 2024 · 2 comments

Comments

@acolytec3
Copy link
Contributor

In working on #3188, @jochem-brouwer and I discovered that if we delete and update package-lock.json in the monorepo root in order to clean up missing or misaligned dependency versions between master and a PR branch, the peer dependencies in package.json marked with optional: true are not automatically installed. This is intended behaviour, based on past discussion from when we switched to vitest because we didn't want to require users of our libraries to have to install large browser binaries necessary for running our browser tests. That said, it's had the unintended consequence of causing headaches when updating package-lock.json because npm i won't install these dependencies automatically which leads to the browser tests failing (and the related CI job).

So, I'm proposing one of two alternatives:

  1. We return the three deps in question (@vitest/browser, webdriverio, and playwright) to the devDependencies so they are always installed unless the user runs npm i --omit=dev. This is certainly the easiest approach since the browser binaries are generally needed for testing and are really part of the development dependency set (though admittedly we don't think about the browser tests much).
  2. We could add a new npm install browser-deps script that manually installs these three dependencies. If we go this route, we need to make sure the CI job for the browser tests runs this script. The tricky bit here is that we would need to remember to run this when we update package-lock.json

I'm not sure if I have a strong preference here but maybe variant 2 is the cleanest (and limits when these extra deps get installed since they would be skipped in the other CI runs)

@holgerd77
Copy link
Member

Thanks for the detailed write-up. Yes, agree, would also have a tendency for a solution in the line of variant 2. 👍

@acolytec3
Copy link
Contributor Author

Option 2 should be addressed in #3188

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

No branches or pull requests

2 participants