Integrate `puppeteer` and `playwright` types

[B4nan]

i was hoping to have those types only at the sdk level, users won't be depending on browser pool directly, right? and if we keep the types in both libraries, we end up with quite heavy duplicates

ideally they would be added to the sdk (the pw types are apparently already there as mentioned on slack), and reused from there somehow (e.g. we could reexport them from sdk and use that export from browser pool)

wdyt?

[B4nan]

actually reading a bit about the type imports, it might be enought just to change the way typings are generated in sdk, so we use type imports for optional peer dependencies (and change to type imports in our packages where pw and puppeteer are currently used). it might require some refactoring (we could only use the type imported symbol for all the signatures, if we need to use the real class somewhere, it can't end up in the .d.ts file)

if that SO answer is right, we might end up with any instead of compilation errors if the dependencies are not installed -- which is just fine, if they are not installed, users can't use them anyway

will verify it locally how it behaves, havent used type imports for this use case yet, but it feels like the best and easies way if its true

[B4nan]

will verify it locally how it behaves, havent used type imports for this use case yet, but it feels like the best and easies way if its true

looks like it fails the same way with the type import, but maybe i'm doing something wrong

[szmarczak]

i was hoping to have those types only at the sdk level, users won't be depending on browser pool directly, right? and if we keep the types in both libraries, we end up with quite heavy duplicates

I thought the SDK could reuse the types from here. I just learned the hard way importing .d.ts files sucks a lot.

At first I was thinking https://github.com/apify/apify-js/blob/master/tools/typescript_fixes.js could be skipped but there's no other way.

actually reading a bit about the type imports, it might be enought just to change the way typings are generated in sdk, so we use type imports for optional peer dependencies

If it works, it will be a game changer. Didn't know about this as well.

[B4nan]

cc @vladfrangu, i first saw type imports in your PRs so maybe you can shed some light on this? :]

btw i recently adopted type imports in here via eslint rule with autofixer, so we can easily enforce them here too

[vladfrangu]

So..if I'm reading this right(please feel free to ask questions or correct me.. I don't entirely see what's up)

• with skipLibCheck on, imports from modules that don't exist should yield any as the type
• type only imports are just a slight sugar on the import syntax that allows typescript to strip out the entire line, or error based on one of the configs..
• I personally don't like that type imports are enforced to be separated from normal imports if they don't have to (consider auto importing...your editor will most likely import types and classes from the same import, which eslint might dislike)

What's the expected outcome from this? 👀 I don't know if I'm entirely up to date with what's up

[szmarczak]

I personally don't like that type imports are enforced to be separated from normal imports if they don't have to

I don't like this as well. Let's hope TS will change this soon.

What's the expected outcome from this?

We generally don't want to force people to install playwright and puppeteer but we refer to their types here. So the types should be optional as well. Just need a nice way to do this.

I think https://github.com/apify/browser-pool/pull/47 and

diff --git a/src/crawlers/browser_crawler.js b/src/crawlers/browser_crawler.js
index dca7108d..cdfdbfc0 100644
--- a/src/crawlers/browser_crawler.js
+++ b/src/crawlers/browser_crawler.js
@@ -257,7 +257,7 @@ export default class BrowserCrawler extends BasicCrawler {
     static optionsShape = {
         ...BasicCrawler.optionsShape,
         // TODO temporary until the API is unified in V2
-        handleRequestFunction: ow.undefined,
+        handleRequestFunction: ow.optional.function,

         handlePageFunction: ow.function,
         gotoFunction: ow.optional.function,
diff --git a/src/crawlers/cheerio_crawler.js b/src/crawlers/cheerio_crawler.js
index 9de350e6..a354683f 100644
--- a/src/crawlers/cheerio_crawler.js
+++ b/src/crawlers/cheerio_crawler.js
@@ -389,7 +389,7 @@ class CheerioCrawler extends BasicCrawler {
     static optionsShape = {
         ...BasicCrawler.optionsShape,
         // TODO temporary until the API is unified in V2
-        handleRequestFunction: ow.undefined,
+        handleRequestFunction: ow.optional.function,

         handlePageFunction: ow.function,
         requestTimeoutSecs: ow.optional.number,

in the SDK should fix this. Keeping you posted.

[vladfrangu]

I personally don't like that type imports are enforced to be separated from normal imports if they don't have to I don't like this as well. Let's hope TS will change this soon.

Not a TS thing! It's probably our lint rules if it enforces that, but TypeScript itself doesn't

We generally don't want to force people to install playwright and puppeteer but we refer to their types here. So the types should be optional as well. Just need a nice way to do this.

It should work if the general types are correct... yeah TS may scream at you if you don't have both installed since we use both (in which case you would sadly need to use skipLibCheck) but that should be alright... keep me up to date here or on slack 🙏

[szmarczak]

Not a TS thing! It's probably our lint rules if it enforces that, but TypeScript itself doesn't

:+1:

TS may scream at you if you don't have both installed since we use both

:+1:

keep me up to date here or on slack

will do