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 types resolution when importing jest types from @jest/globals #602

Merged
merged 3 commits into from
Oct 22, 2024

Conversation

mulekick
Copy link
Contributor

@mulekick mulekick commented Oct 19, 2024

Summary

This pull request contains :

  1. An additional module declaration that adds support for expect-puppeteer matchers types resolution when jest types are imported from @jest/globals (issue 601).
  2. As a result, installing @types/jest is no longer mandatory and the project is now 100% compliant to jest official recommendations.
  3. Edits of the main README and the jest-environment-puppeteer README to describe how to set up with typescript in a more detailed way.
  4. An edit of the expect-puppeteer README to adhere to the current API and point to the new puppeteer docs.
  5. Removals of the expect-puppeteer imports in the test files that do not use expect.
  6. A few upgrades of deprecated methods on the puppeteer API (see toClick.ts).

Note : due to the way typescript resolves modules, the community-provided types for jest-puppeteer will be prevalent if still installed despite being outdated. As a result, I've added a note in the README to recommend uninstalling them when upgrading to >=10.1.2.

Test plan

  1. No breaking changes or regressions should occur since the implementation was left untouched, only types declarations were changed. Note though that a (breaking?) change was introduced in 10.1.2 since the types resolution for the jest-environment-puppeteer globals requires importing jest-puppeteer explicitly when not using the deprecated pre v8.0.0 types. As a result, I don't know if it commends a new minor let major version since it only affects the DX, but I think it's worthy of mentioning.

  2. Test suite passes 100%.

Test suite passes 100%

  1. When testing the minimal repo with the following dependencies (notice that @types/jest is not installed) :

current dependencies

  1. Typescript complains, types resolution does not work

Types resolution fails

  1. Then changing dependencies to point to my fork and npm i :

updated dependencies

  1. Typescript is ok and types resolution works again 👍

Types resolution works

  1. I've also tested that types resolution works the same way when reinstalling @types/jest.

@gregberge gregberge merged commit e5b2e1a into argos-ci:main Oct 22, 2024
3 checks passed
@gregberge
Copy link
Member

@mulekick thanks for that

@mulekick
Copy link
Contributor Author

@gregberge You're welcome ! Maybe you should close or update issue 568 as well since it points to the problem that was addressed here.

@mulekick
Copy link
Contributor Author

mulekick commented Oct 24, 2024

@gregberge I think you should unpublish 10.1.3 asap and rollback to 10.1.2 : my tests were not complete, and after conducting further tests today I realized that the expect implementation imported from @jest/globals does not support the puppeteer matchers even if the types resolution is ok, which causes any test suite relying on them to break.

The error can be reproduced in the minimal repo

Edit : there is indeed a pattern that allows extending the @jest/globals implementation, but it is too restrictive (matchers always must return objects with the same signature, errors are ignored) and thus would break some functionality and require a significant refactoring of the entire codebase just to accomodate.

@gregberge
Copy link
Member

Working on it: #604

@mulekick
Copy link
Contributor Author

@gregberge I've opened a PR to reinstate everything that was lost in the process minus the broken @jest/globals import 👍

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.

2 participants