Move puppeteer to peer dependencies

[ivomurrell]

question: why is this necessary? we require puppeteer explicitly here

[jkerr321]

question: why is this necessary? we require puppeteer explicitly here

@ivomurrell it's not necessary, I just thought it was best practice now that peerDependencies are available.

So, if a package is directly required it shouldn't be listed as a peerDependency? I'm confused about, e.g. Tool Kit which has jest-cli as a peerDependency and requires it in the code here (const jestCLIPath = require.resolve('jest-cli/bin/jest') - or is that a different case because the whole package is not being required, rather just the executable on that file path is?

[juanSanchezAlcala]

There are newer versions that doesn't work because they have some bugs that are raised using this repo. The latest version that i could manage to run without errors is 18.1.0. So to avoid other developers to come with the same issue i would pin on dotcom-tool-kit/n-test the puppeteer version to 18.1.0

[ivomurrell]

@ivomurrell it's not necessary, I just thought it was best practice now that peerDependencies are available.

So, if a package is directly required it shouldn't be listed as a peerDependency? I'm confused about, e.g. Tool Kit which has jest-cli as a peerDependency and requires it in the code here (const jestCLIPath = require.resolve('jest-cli/bin/jest') - or is that a different case because the whole package is not being required, rather just the executable on that file path is?

I haven't answered this question all day because I was hoping someone else would answer it for me as I'm not sure how to tackle it 😁

My primary reference for when to use peer dependencies is this excellent blog post by Domenic Denicola. In it, he explains that the primary use case for peer dependencies is for plugins – where a package is meant to be used in combination with a 'host' package, but doesn't import from the host directly. That's useful in a few of our Tool Kit packages, such as the husky plugin which installs husky configuration into your package.json but doesn't depend on husky explicitly, instead depending on a specific major version of husky that consumes that config in the way we expect. However, you're right that the jest plugin instead require's and runs Jest directly. I'm not sure that it does actually need to be a peer dependency in that case? I wonder if our decisions on whether plugins should use peer dependencies (particularly mine!) have been clouded by the fact that they are called 'plugins'? So I think it might be Tool Kit that's wrong here, not n-test? Like Kara says, there are other uses for peer dependencies, like enforcing a flattened tree à la Bower, but I don't think that applies here. Soooo I don't think there's any reason for this to be a peer dependency? We're running puppeteer within n-test itself, rather than setting up some configuration for a separate puppeteer instance to consume.

There are newer versions that doesn't work because they have some bugs that are raised using this repo. The latest version that i could manage to run without errors is 18.1.0. So to avoid other developers to come with the same issue i would pin on dotcom-tool-kit/n-test the puppeteer version to 18.1.0

It makes sense to pin it to 18.1.0 instead of ^18.1.0 then, though we don't need to make it a peer dependency to do that.

[jkerr321]

Thank you everyone for the feedback @ivomurrell @apaleslimghost 🙌 ❤️

I am going to close this as I think puppeteer can stay in dependencies, but I learnt something along the way so thank you for that!