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

feat: Add typings #58

Merged
merged 3 commits into from
Jul 21, 2022
Merged

feat: Add typings #58

merged 3 commits into from
Jul 21, 2022

Conversation

anishkny
Copy link
Owner

@anishkny anishkny commented Jul 20, 2022

Closes #53

@anishkny
Copy link
Owner Author

@kachkaev thanks for the suggestion in #53.

I am not an expert in TS. Please take a look at this PR and suggest any changes as appropriate.

PS: Thanks for the kind words and gentle tone in asking for enhancements. Really appreciate it!

Copy link
Collaborator

@kachkaev kachkaev left a comment

Choose a reason for hiding this comment

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

Hi @anishkny, thanks for this PR! I tried your diff in my experiment (blockprotocol/blockprotocol#447) and it worked!

The only thing I overlooked in #53 was the scope of exports. I thought playwright-test-coverage was re-exporting everything from @playwright/test, but it was actually just test and expect.

My diff suggestion should do the job.

index.d.ts Outdated Show resolved Hide resolved
@kachkaev
Copy link
Collaborator

kachkaev commented Jul 21, 2022

We can consider switching back to export * later if we re-export everything from @playwright/test. Doing so will allow us to write this, for instance:

import { devices } from "playwright-test-coverage"

Direct imports from @playwright/tests can be then completely banned for consistency. That’s is a general idea, I’m not suggesting to consider it in this PR.

Co-authored-by: Alexander Kachkaev <alexander@kachkaev.ru>
@kachkaev
Copy link
Collaborator

Thanks for addressing my comment @anishkny, the diff LGTM! 👏

@anishkny anishkny merged commit e8348c5 into main Jul 21, 2022
@anishkny anishkny deleted the feat/add-typings branch July 21, 2022 17:13
@anishkny
Copy link
Owner Author

Shipped in v1.1.0

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.

TypeScript typings
2 participants