playwright-core vs playwright as peerDependency

[thernstig]

Currently I have to install both playwright and playwright-core in my package.json in order to run expect-playwright.

If I compare the subdependencies of playwright and playwright-core, they are identical. How come playwright could not be listed as the peer dependency instead?

[mxschmitt]

We need playwright-core to expose types for the users. Having it installed as a normal dependency in expect-playwright does not always work, since we have to update it all the time and hold it in sync with the install playwright or playwright-* version.

playwright-core is currently a peerDependency:

expect-playwright/package.json

Lines 14 to 16 in 6a1d637

	"peerDependencies": {
	"playwright-core": "^1.9.1"
	},

Or do you mean on Playwright side?

[thernstig]

I meant why is this not ok?

 "peerDependencies": { 
   "playwright": "^1.9.1" 
 }, 

The current solution with requiring playwright-core instead of playwright as the peerDependencies.

Any user using expect-playwright otherwise have to have both playwright and playwright-core in their package.json.

[mxschmitt]

This example would install playwright all the time which installs all the browsers on all the users environments. We want to prevent that and when a user is installing playwright-chromium that only chromium gets installed.

Yep this is unfortunately the pitfall that the user needs to have two packages of playwright there but unfortunately the only solution to workaround that problem.

[thernstig]

Isn't it necessary though for a user to already have the playwright package? I mean could I just have expect-playwright and playwright-core in my package.json? I assume you are saying that is the case (although uncommon).

But I recon that 99% of users already have playwright installed so if you had that as peerDependencies I'd think very few users would have trouble with extra installs. Besides browsers are cached :) I might be missing something still though here.

(I'll admit techincally it is more correct for you to have playwright-core but I just thought it'd be more pragmatic to use playwright instead).

[mskelton]

Jumping in as a user who uses playwright-chromium, having the peer dep as playwright would not be ideal especially in CI where browsers are downloaded fresh. The current behavior of using playwright-core as the peer dep is correct.

[thernstig]

Is there a reason that PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD (reference) is not suitable? Maybe it doesn't work together with manually downloading particular binaries with playwright-chromium etc? In npm 7 peer dependencies are also automatically installed if not listed as a dependency already.

[mskelton]

@thernstig Using a flag such as PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD would not work for two reasons:

1. It would require consumers to add this flag to their environment which should not be required for the library to work out of the box.
2. Users who are using playwright-chromium, playwright-firefox, etc. still want browsers downloaded, they just want a specific browser downloaded rather than all the browsers downloaded.

[thernstig]

1. That is not true for all users, as I'd recon the majority of the users want to download all browsers by default. Which also means most users install the playwright package by default. Having to set PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD is what few users have to do.
2. Maybe there should be a PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD_CHROMIUM, PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD_FIREFOX, PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD_WEBKIT as well.

Maybe I am wrong here. But the problem I find is that a user that installs expect-playwright and do not not install playwright (or the browser-specific versions), have no browsers at all. Or is that incorrect? If so just installing playwright-core will not work. Or maybe I am completely incorrect there? If I am not, then just installing playwright-core will not make it possible to run any tests without the browsers. In addition, as I suspect most users do already have playwright installed, having to list playwright-core as an additional dependency is confusing to users as you'd need to update both that version and the normal playwright version when upgrading, which can be missed.

Mind you I am just trying to find the best option here for the majority of users. And I might be completely incorrect in my assumptions and recommendations, but I have not see other packages doing it like this before hence why it jumped at me as a bit "off".

[mskelton]

That is not true for all users, as I'd recon the majority of the users want to download all browsers by default. Which also means most users install the playwright package by default. Having to set PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD is what few users have to do.

I'll grant this point any day. Most users definitely use the playwright package, however those who use the specific browser packages should also be supported.

Maybe there should be a PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD_CHROMIUM, PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD_FIREFOX, PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD_WEBKIT as well.

Definitely an option, but requiring users to set environment variables to use a package is not a good experience.

Maybe I am wrong here. But the problem I find is that a user that installs expect-playwright and do not not install playwright (or the browser-specific versions), have no browsers at all. Or is that incorrect?

You are correct, if a user installs expect-playwright and doesn't install playwright or a browser specific variant won't get any browsers. That said, this is not a problem as users who are consuming expect-playwright should be installing the version and type of playwright package they would like. This is the reason why it's a peer dependency in the first place and not a regular dependency. For npm 7 users, this entire thread isn't really that impactful since peer deps are automatically installed, so playwright-core will be automatically installed. However, if the peer dep was playwright, now npm 7 users will have playwright installed automatically even if they only want to use a browser variant.

The really unfortunate part of this whole conversation is that the only usage of playwright-core in this repo is for TypeScript definitions. If Playwright published their TypeScript definitions as a standalone package, that would reduce a lot of this friction in a lot of projects like this one.

[mskelton]

@mxschmitt Just got to thinking, but is there any downsides to installing playwright-core as a regular dependency rather than a peer dep? Or does it not play nicely if there are multiple versions?

[thernstig]

Installing it as a regular dependency still gets into the trouble of differeing versions between users normal playwright package.

I am still a believer that playwright should be the peerDepdency as it means users can update playwright independent of expect-playwright.

Definitely an option, but requiring users to set environment variables to use a package is not a good experience.

This is not required by most users. Actually, it is not required by any user. I'd say your use case is the outlier here and there is a much bigger problem with a missmatch between users playwright and playwright-core.

[mskelton]

Installing it as a regular dependency still gets into the trouble of differeing versions between users normal playwright package.

Right, but thinking more about it, that should be fine since expect-playwright only needs the type definitions for whatever lowest supported version of playwright. If a new version comes out that has new features, it won't be harmful if expect-playwright has an older version of the types.

This is not required by most users. Actually, it is not required by any user.

How is it not required by any user? It would be for users like me.

I'd say your use case is the outlier here and there is a much bigger problem with a missmatch between users playwright and playwright-core.

This repository is actually another place that doesn't use playwright but instead uses playwright-chromium.

https://github.com/playwright-community/expect-playwright/blob/master/package.json#L22-L23