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

chore(jest-jasmine2): remove usage of `Function` type #10216

Merged
merged 2 commits into from Jul 5, 2020

Conversation

@G-Rath
Copy link
Contributor

G-Rath commented Jun 28, 2020

Summary

Improves the types for jest-jasmine2 to remove banned types (specifically Function & object) as part of #10177.

The only two files that have banned types (well, type: Function) are:

  • errorOnPrivate: I suspect that Function might actually be the "right" type to use here, but more importantly it's usage is for a passthrough parameter to ErrorWithStack which comes from jest-utils, so the correct type will be whatever replaces the Function usage in that package.

  • jasmineAsyncInstall: I've almost got this typed, but the remaining errors point towards a type being incorrect somewhere that I can't pin down. I think there's a bit of critical understanding that would make it all click that I'm missing.

    I've included the work in this commit, so CI will fail, but am happy to revert that section to get the rest of the changes landed.

Test plan

See if the tests pass :D

@G-Rath G-Rath force-pushed the G-Rath:improve-jest-jasmine2-types branch from 1486aee to 103bf3d Jun 28, 2020
@G-Rath G-Rath force-pushed the G-Rath:improve-jest-jasmine2-types branch from 103bf3d to b58c559 Jun 28, 2020
@G-Rath G-Rath force-pushed the G-Rath:improve-jest-jasmine2-types branch from 3eaae79 to 0377495 Jul 4, 2020
@SimenB
SimenB approved these changes Jul 5, 2020
env: Jasmine['currentEnv_'],
) {
return function <T>(
fn: Function | (() => Promise<T>) | GeneratorFunction | undefined,
fn:
| ((done: DoneFn) => void | PromiseLike<T>)

This comment has been minimized.

@SimenB

SimenB Jul 5, 2020 Collaborator

It should be an error to both take a done function and return a promise

This comment has been minimized.

@G-Rath

G-Rath Jul 5, 2020 Author Contributor

This will cause some major type problems, as everywhere else is expecting only (done: DoneFn) => void.

For the record, by unioning the functions TS can't support both of them when doing .call, so it favors (done: Done) => void, meaning that returnValue.then is invalid as it has type void.

This can be resolved by using an interface:

interface OneOrTheOther {
  (done: DoneFn): void;
  (): PromiseLike<void>;
}

This then takes us back to where we were before, but this time it's the opposite: TS is complaining asyncJestTest doesn't fit because it's of type (done: DoneFn) => void instead of OneOrTheOther 😬

This comment has been minimized.

@SimenB

SimenB Jul 5, 2020 Collaborator

😞

@G-Rath G-Rath force-pushed the G-Rath:improve-jest-jasmine2-types branch from 0377495 to b8198c8 Jul 5, 2020
@SimenB SimenB merged commit c2d4aa5 into facebook:master Jul 5, 2020
21 of 22 checks passed
21 of 22 checks passed
cleanup-runs
Details
Running TypeScript compiler & ESLint
Details
Node v10.x on ubuntu-latest
Details
Node v10.x on macOS-latest
Details
Node v10.x on windows-latest
Details
Node v12.x on ubuntu-latest
Details
Node v12.x on macOS-latest
Details
Node v12.x on windows-latest
Details
Node v13.x on ubuntu-latest
Details
Node v13.x on macOS-latest
Details
Node v13.x on windows-latest
Details
Node v14.x on ubuntu-latest
Details
Node v14.x on macOS-latest
Details
Node v14.x on windows-latest Node v14.x on windows-latest
Details
ci/circleci: test-jest-circus Your tests passed on CircleCI!
Details
ci/circleci: test-node-10 Your tests passed on CircleCI!
Details
ci/circleci: test-node-12 Your tests passed on CircleCI!
Details
ci/circleci: test-node-13 Your tests passed on CircleCI!
Details
ci/circleci: test-node-14 Your tests passed on CircleCI!
Details
ci/circleci: test-or-deploy-website Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
facebook.jest #20200705.6 succeeded
Details
@SimenB
Copy link
Collaborator

SimenB commented Jul 5, 2020

Thanks!

@G-Rath G-Rath deleted the G-Rath:improve-jest-jasmine2-types branch Jul 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.