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

fix: type errors when playwright is not installed #1637

Merged
merged 1 commit into from
Oct 30, 2022

Conversation

szmarczak
Copy link
Contributor

@szmarczak szmarczak commented Oct 27, 2022

Fixes #1610
Closes #1629

@szmarczak
Copy link
Contributor Author

Do we also need

} else if (
// playwright/puppeteer import
line.match(/^([^']+)'(playwright|puppeteer)'/) ||
// proxy-per-page reexport of puppeteer
line.match(/: Puppeteer\.\w+/) ||
// don't ask me why, but this one is needed too ¯\_(ツ)_/¯
line.match(/^export interface (PlaywrightHook|PuppeteerHook)/)
) {
output.push('// @ts-ignore optional peer dependency');
output.push(line);
changed = true;
?

Doesn't import type return any when a type does not exist? So // @ts-ignore shouldn't be necessary.

@vladfrangu
Copy link
Member

Does this supersede #1629? 👀

@vladfrangu
Copy link
Member

Doesn't import type return any when a type does not exist?

Nope, it'll still throw an error like importing without the type modifier 😓

@szmarczak
Copy link
Contributor Author

szmarczak commented Oct 27, 2022

Nope, it'll still throw an error like importing without the type modifier sweat

Shouldn't we use await import in that case? OTOH it may not work in cases where synchronous work is done first and that exposed via a synchronous function as well. That could be launching a browser for example (I think it's async though). However in ESM optional deps should be loaded via await import.

Edit: yep importing non-installed package throws. That's gonna be a separate task I guess.

@vladfrangu vladfrangu changed the title fix: playwright ts support fix: type errors with missing Playwright install from meta package Oct 29, 2022
@vladfrangu
Copy link
Member

Tested this locally and it works 🎉

@szmarczak szmarczak changed the title fix: type errors with missing Playwright install from meta package fix: type errors when playwright is not installed Oct 30, 2022
@szmarczak szmarczak merged commit de9db0c into master Oct 30, 2022
@szmarczak szmarczak deleted the fix-ts-playwright branch October 30, 2022 19:41
* This is required when using optional dependencies.
* Importing a type gives `any`, but `Parameters<any>` gives `unknown[]` instead of `any`
*/
export type SafeParameters<T extends (...args: any) => any> = unknown[] extends Parameters<T> ? any : Parameters<T>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should move this to @crawlee/types (probably), and mark it as @internal (definitely)

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

Successfully merging this pull request may close these issues.

Puppeteer template project TS build fails
3 participants