fix: fix exported types

[timocov]

What kind of change does this PR introduce? bugfix

Did you add tests for your changes? No

If relevant, did you update the documentation? Not relevant

Summary

You cannot export functions if actually you export a class (methods of a class != separate functions).

Fixes #7529

Does this PR introduce a breaking change?

To use launch method (and any other methods from Puppeteer class) now you have to import puppeteer via import default export, i.e. import puppeteer from 'puppeteer'.

Other information

[timocov]

Interesting. puppeteer exports its instance as a default export, but in some types tests the test files rely on the knowledge that the exports are actually commonjs. So we have common types which are working for both commonjs and esm, but they could work for esm only since export default doesn't exist in commonjs. At least from my understanding it looks so.

It seems that the export of the instance should be kind of export = or export default depending on the module type, but as soon as types are only one for all module types we need to choose one of them?

@jschfflr could you please help with that?

[timocov]

@whimboo @connorjclark @jackfranklin @OrKoN maybe you can help with ^^ please?

[OrKoN]

Hey, thanks for the PR. Do you know if this will have any impact on CommonJS consumers? Do you see a way to fix the types without breaking changes for the consumers? Could you please rebase so that we can run CI?

[timocov]

Could you please rebase so that we can run CI?

Done (but tbh I didn't check it locally since I was on a windows pc right now and it might just fail because of that).

Do you see a way to fix the types without breaking changes for the consumers?

I didn't see any way to do this. The types are incorrect and should be fixed so it would be a breaking change anyway.

Do you know if this will have any impact on CommonJS consumers?

Well, I think this is quite similar to what I asked in #7742 (comment) because I'm not too sure how it works internally.

[OrKoN]

Thanks! I was under impression that src/api-docs-entry.ts is only used for the documentation generation so should not actually be shipped (at least there are comments indicating that). I will double check next week. I think comment https://github.com/puppeteer/puppeteer/blob/main/cjs-entry.js#L18 and cjs-entry.js might be causing the issue.

[timocov]

@OrKoN have you had a chance to double check it? 🙂

[OrKoN]

@timocov sorry, I have not managed yet. But it looks like the PR is failing CI checks anyway?

[timocov]

But it looks like the PR is failing CI checks anyway?

@OrKoN that's true, but to fix them we need to answer questions from #7742 (comment) I believe. It looks like a problem is still here, but it just works in some cases and no one mentioned it before. Probably it might be solved by using exports map (and provide different types files for different type of importing a package, but I'm not sure if tsc supports it yet. ¯_(ツ)_/¯

[jackfranklin]

Interesting. puppeteer exports its instance as a default export, but in some types tests the test files rely on the knowledge that the exports are actually commonjs. So we have common types which are working for both commonjs and esm, but they could work for esm only since export default doesn't exist in commonjs. At least from my understanding it looks so.

Reaching back to when I did the work to setup this build, I think this is because when we first released the types we had issues where it did work for CJS users, or ESM users, but not both. So this test suite exists to ensure that our type configuration supports all users and their setups. Long term I'd like to drop CJS and move to ESM, but that's a separate discussion.

RE api-docs-entry.ts, I think that should only be used by the code that takes all the TSDoc comments and generates the docs, and not as part of the main types that are used by the compiler when checking the code. But I'm reaching back to work that was done 12 months ago and it's not the most fresh in my memory, so I'd need to dig into the setup to remind myself.

[jrandolf]

This does not seem relevant anymore. A workaround is to use import * as puppeteer.

Closing this as this PR is stale.

[OrKoN]

I think we still need to address that so that the types match the documentation.

[jrandolf]

@OrKoN Wouldn't this affect people who already import using import * as puppeteer? This would be a breaking change (which could be fine since we have a major version coming up).

[OrKoN]

yeah, it would be a breaking change. I think we need to do the following:

1. make sure that our types match our implementation across CJS and ESM builds (currently, it's not the case)
2. figure out the role of src/api-docs-entry.ts as it appears it was meant to be the aid for the documentation generator but it looks like it affects types we ship.

[jrandolf]

Closed in favor of #8493