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

update Matchers to Matchers<R,T> #235

Conversation

Shadowstep33
Copy link
Contributor

What

@types/jest changed Matchers to be Matchers<R,T> this change is just supporting that

Why

npm build will fail for TS projects with both jest-extended@0.11.2 and @types/jest@24.0.20

Notes

Housekeeping

  • Unit tests
  • Documentation is up to date
  • No additional lint warnings
  • Typescript definitions are added/updated where relevant

@Shadowstep33
Copy link
Contributor Author

This is my first PR to this project. Not sure if I need to manually bump V and I also had some issues w/ tests but anecdotally I changed my local node_modules/jest-extended to reflect this change and the build worked

@Shadowstep33
Copy link
Contributor Author

Additionally, for those getting this error


node_modules/@types/jest/index.d.ts:639:15 - error TS2428: All declarations of 'Matchers' must have identical type parameters.

639     interface Matchers<R, T> {
                  ~~~~~~~~

node_modules/jest-extended/types/index.d.ts:5:13 - error TS2428: All declarations of 'Matchers' must have identical type parameters.

5   interface Matchers<R> {
              ~~~~~~~~

and needing an immediate fix, you can revert to @types/jest@24.0.19 safely. This bug was introduced a few days ago

@codecov-io
Copy link

Codecov Report

Merging #235 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #235   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         109    109           
  Lines         554    554           
  Branches       97     97           
=====================================
  Hits          554    554

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1329f78...6826c91. Read the comment docs.

@illBeRoy
Copy link

Thanks for taking action to solve this! I've just run into this issue as well, and had to disable lib check for now...
Maintainers - any idea when this is going to be merged?

@gcangussu
Copy link

This same problem is blocking us from updating to the latest @types/jest
Can this fix be released quickly? Probably a better fix would use that new variable T somewhere in the definitions, but this could take more time, and could be done later without losing any type safety we already don't have.

@InExtremaRes
Copy link

InExtremaRes commented Nov 5, 2019

I made the same change locally, but it is not enough though; the rules await-promise and no-void-expression of TSLint fire out.

The issue is that new jest typings also explicitly makes R to be void or Promise<void> instead of inferring it from the actual type (this is now T); so toReject(): R and toResolve(): R now both return void thus the failing TSLint rules.

await expect(promise).toReject(); // type is void, not Promise<R>

Types of jest itself change the argument from R to Promise<void> after a .resolves. or .rejects. but jest-extended does not use those for toReject() and toResolve().

BTW is this project still under maintenance?

@antonyoneill
Copy link

@mattphillips are you still maintaining this project?

I appreciate the upstream types have changed under your feet but this PR has been open for almost a month now and is holding upgrades back in my app.

@Seally
Copy link

Seally commented Nov 24, 2019

Note to people who've been hit by this issue, check out if patch-package works out for you. It is in no way a solution (and adds quite a few dependencies) but it did help alleviate the problem for me while allowing the Jest type defs to be updated.

@gcangussu
Copy link

@Seally You can also add a file jest-extended.d.ts with the correct type declarations to you project root and Typescript will pick it up. No need to patch-package.

@effrenus
Copy link

@mattphillips Thanks for the package, but now it incompatible with newer versions of @types/jest. Could you watch this PR? 🙏

@InExtremaRes
Copy link

@Seally You can also add a file jest-extended.d.ts with the correct type declarations to you project root and Typescript will pick it up. No need to patch-package.

@gcangussu Have you managed to make it work? I've tried that (including the use of types and similar) but doesn't work properly; I don't know if I'm making something wrong. TypeScript does read my jest-extended.d.ts since I see the effect of some changes but it is still reading the original file from node_modules so the error about different parameters for Matcher is still there. I even opened a question in SO some time ago.

@thoraj
Copy link

thoraj commented Jan 6, 2020

"Overriding" the types using jest-extended.d.ts worked for me.
But is there an ETA of getting this fixed so we don't have to use that trick?

@gcangussu
Copy link

@gcangussu Have you managed to make it work? I've tried that (including the use of types and similar) but doesn't work properly; I don't know if I'm making something wrong. TypeScript does read my jest-extended.d.ts since I see the effect of some changes but it is still reading the original file from node_modules so the error about different parameters for Matcher is still there. I even opened a question in SO some time ago.

@InExtremaRes You need to make sure you are adding it to your project on the tsconfig.json. Look into files or include/exclude properties. It should replace the module declared on this package. At least it does here 😕.

@akahan
Copy link

akahan commented Jan 12, 2020

@Seally You can also add a file jest-extended.d.ts with the correct type declarations to you project root and Typescript will pick it up. No need to patch-package.

@gcangussu Have you managed to make it work? I've tried that (including the use of types and similar) but doesn't work properly; I don't know if I'm making something wrong. TypeScript does read my jest-extended.d.ts since I see the effect of some changes but it is still reading the original file from node_modules so the error about different parameters for Matcher is still there. I even opened a question in SO some time ago.

You need to explicitly specify the order of @types directories in typeRoots option. For example, if you have folders «node_modules/@types» and «src/@types», the first should be «src/@types».
"typeRoots": ["src/@types", "node_modules/@types"], "exclude": ["node_modules/jest-extended/types/*"]

@ivoreis
Copy link

ivoreis commented Jan 20, 2020

👋 @stephtr Can we merge this PR into master or is there any reason why is being blocked?

@stephtr
Copy link
Member

stephtr commented Jan 20, 2020

Thanks for bringing it to my attention and sorry, that nobody cared about it for such a long time.
@connectdotz Who is responsible for releasing new versions of jest-extended?
Also worth taking a look at: Adding typechecking.

@connectdotz
Copy link

Who is responsible for releasing new versions of jest-extended?

Hmm, seems like it has always been @mattphillips, who has published the latest release on Jul 9, 2019. Not sure if he has other maintainers to help out... Hopefully, he will check in soon 🤞

@kbradl16
Copy link

kbradl16 commented Jan 21, 2020

I wouldn’t get your hopes up, this project is dead... Someone should clone and allow more than 1 maintainer for it to be successful.

#205

@mattphillips
Copy link
Member

Hey all sorry for the delay in coming to this!!!

It looks to me like @types/jest types are actually incorrect.

I've opened a PR (DefinitelyTyped/DefinitelyTyped#41766) to fix them which should mean this change is redundant.

In the meantime though I will release this a fix for this

@mattphillips mattphillips merged commit f5571a1 into jest-community:master Jan 21, 2020
@mattphillips
Copy link
Member

Fix released in: https://www.npmjs.com/package/jest-extended/v/0.11.3

@InExtremaRes
Copy link

InExtremaRes commented Jan 21, 2020

@mattphillips Thank you for merging, but as I mentioned previously this change is not enough. The types of toResolve() and toReject() should be now

toResolve(): Promise<R>; // or maybe Promise<void> directly
toReject(): Promise<R>;

instead of just returning R.

The change comes from jest now wrapping the type in a Promise after a .resolves or .rejects, but since this library use neither of those the return type of toResolve() is void. In our case this means a rule of tslint fires out:

Invalid 'await' of a non-Promise value. (await-promise) 

@mattphillips
Copy link
Member

Hey @InExtremaRes sorry I missed your comment - just published:

toResolve(): Promise<R>;
toReject(): Promise<R>;

in: https://www.npmjs.com/package/jest-extended/v/0.11.4

Let me know if there are any other issues :)

@InExtremaRes
Copy link

@mattphillips Thanks a lot

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.

None yet