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(jest): add mocked property #57775

Closed
wants to merge 2 commits into from

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Dec 19, 2021

Please fill in this template.

Select one of these and delete the others:

If changing an existing definition:

Resolves #57716

@typescript-bot typescript-bot added this to Needs Author Action in New Pull Request Status Board Dec 19, 2021
@G-Rath
Copy link
Contributor Author

G-Rath commented Dec 19, 2021

I'm not super happy about having another dependency + WebStorm doesn't seem the happiest about this vs using mocked from ts-jest, but it's passing CI so let's ship it 🤷

(when we eventually all switch to ESM we'll be importing jest from @jest/globals anyway which will make WebStorm happy if it's not improved before then)

@G-Rath G-Rath marked this pull request as ready for review December 19, 2021 03:29
@typescript-bot
Copy link
Contributor

typescript-bot commented Dec 19, 2021

@G-Rath Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

You can test the changes of this PR in the Playground.

Status

  • ✅ No merge conflicts
  • ✅ Continuous integration tests have passed
  • 🕐 Most recent commit is approved by a DT maintainer

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.


Diagnostic Information: What the bot saw about this PR
{
  "type": "info",
  "now": "-",
  "pr_number": 57775,
  "author": "G-Rath",
  "headCommitOid": "e615bc7b620a06f041aa7948aac5319440905d4a",
  "lastPushDate": "2021-12-19T03:12:08.000Z",
  "lastActivityDate": "2021-12-20T10:12:10.000Z",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "jest",
      "kind": "edit",
      "files": [
        {
          "path": "types/jest/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/jest/jest-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/jest/package.json",
          "kind": "package-meta-ok"
        }
      ],
      "owners": [
        "NoHomey",
        "jwbay",
        "asvetliakov",
        "alexjoverm",
        "epicallan",
        "ikatyang",
        "wsmd",
        "JamieMason",
        "douglasduteil",
        "ahnpnl",
        "UselessPickles",
        "r3nya",
        "hotell",
        "sebald",
        "andys8",
        "antoinebrault",
        "gstamac",
        "ExE-Boss",
        "quassnoi",
        "Belco90",
        "tonyhallett",
        "ycmjason",
        "devanshj",
        "pawfa",
        "regevbr",
        "gerkindev",
        "domdomegg"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [],
  "mainBotCommentID": 997323297,
  "ciResult": "pass"
}

@typescript-bot typescript-bot moved this from Needs Author Action to Needs Maintainer Review in New Pull Request Status Board Dec 19, 2021
@typescript-bot
Copy link
Contributor

🔔 @NoHomey @jwbay @asvetliakov @alexjoverm @epicallan @ikatyang @wsmd @JamieMason @douglasduteil @ahnpnl @UselessPickles @r3nya @Hotell @sebald @andys8 @antoinebrault @gstamac @ExE-Boss @quassnoi @Belco90 @tonyhallett @ycmjason @devanshj @pawfa @regevbr @GerkinDev @domdomegg — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

@k-rajat19
Copy link
Contributor

k-rajat19 commented Dec 19, 2021

Hi @G-Rath I have opened a new PR regarding this issue can you please look into 🙂
#57776

@@ -73,6 +73,7 @@ type ExtractEachCallbackArgs<T extends ReadonlyArray<any>> = {
];

declare namespace jest {
const mocked: typeof import('jest-mock').mocked;
Copy link
Contributor

Choose a reason for hiding this comment

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

since we adding jest-mock dependency here
should we need to import all types/functions exported by jest-mock like this. No ?
so that it looks consistent :)
or it doesn't matter

Copy link
Contributor Author

@G-Rath G-Rath Dec 19, 2021

Choose a reason for hiding this comment

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

For now it doesn't matter as this is a new API whereas the existing types are slightly different from what's provided in jest-mock so need to be checked first and that's not as important as typing the new api.

Copy link
Contributor

@SimenB SimenB Dec 20, 2021

Choose a reason for hiding this comment

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

won't this break for people who have e.g.

function doSomething(jest.Mock<something, somethingElse>) {
}

doSomething(jest.mocked({}));

?

(Long term I want @types/jest to import all of its things from Jest itself (#44365), but as long as that's not the case, mixing seems less than ideal?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What you've provided isn't actually valid because jest.Mock is actually sort of poorly named jest.MockedFunction, where as jest.mocked({}) returns jest.MockedObject.

However no it won't because the types should all be compatible.

here's some code showcasing this
import { mocked as tsJestMocked } from 'ts-jest/utils';
// import { jest } from '@jest/globals';
import * as MockTypes from 'jest-mock';
import * as jestMock from 'jest-mock';

// type MockTypes = jest;
// type MockTypes = typeof jestMock;
const mocked = jestMock.mocked;
// const mocked = jest.mocked;
// const mocked = tsJestMocked;

class Greeter {
  private readonly _greeting: string;

  public constructor(greeting: string) {
    this._greeting = greeting;
  }

  public get greeting() {
    return this._greeting;
  }

  public sayHello(target: string) {
    console.log(`${this._greeting} ${target}`);
  }
}

interface Point {
  x: string;
  y: number;
}

const makePoint = (x: string, y: number): Point => {
  return { x, y };
};

declare function doSomethingWithMockedFn(
  fn: jest.Mock<Point, [string, number]>
): void;
declare function doSomethingWithMockedMakePoint(
  fn: jest.Mocked<typeof makePoint>
): void;

declare function doSomethingWithMockedObject(fn: jest.Mocked<Point>): void;

declare function doSomethingWithMockedClass(
  fn: jest.MockedClass<typeof Greeter>
): void;

doSomethingWithMockedFn(mocked(makePoint));
doSomethingWithMockedMakePoint(mocked(makePoint));
doSomethingWithMockedObject(mocked({ x: '', y: 1 }));
doSomethingWithMockedClass(mocked(Greeter));

Regardless of what combination you use (e.g. types from jest-mocked + mocked from the jest global, everything from jest-mocked, everything from ts-jest, renaming the jest.<type> to be MockTypes.<type>, etc) the errors and passes are the same.

The first and last doSomething method calls have errors because of type issues (which basically boil down to the conditional typing of mocked resulting in a more accurate type than what's being used) while the middle two calls don't.


Long term I want @types/jest to import all of its things from Jest itself (#44365), but as long as that's not the case, mixing seems less than ideal?

That was part of my goal here, as we're already mixing types with jest-diff and pretty-format - but I didn't want to change the existing types in this PR because of the size of that work.

However looking at it with fresh eyes, it'll be a better IDE experience anyway to go with #57776 so let's do that.

https://github.com/DefinitelyTyped/DefinitelyTyped/pull/57776/files#r772003152

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(the problem with #57776 is it is mixing types too, but it's more subtle because they have slightly different names)

@G-Rath G-Rath closed this Dec 20, 2021
@G-Rath
Copy link
Contributor Author

G-Rath commented Dec 20, 2021

Closed in favor of #57776

@typescript-bot typescript-bot removed this from Needs Maintainer Review in New Pull Request Status Board Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants