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: improve types #42

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: improve types #42

wants to merge 4 commits into from

Conversation

hongaar
Copy link

@hongaar hongaar commented Dec 15, 2022

Various improvements to the TypeScript types:

  • Add TestOptions.only
  • Add done param to TestFn
  • Update test ReturnType to Promise<void>
  • Remove ItContext and replace param with in it fn type with done
  • Add exported hooks
  • Change TestContext.test to typeof test
  • Added TestContext.runOnly, TestContext.beforeEach, TestContext.afterEach, TestContext.name
  • Formatfile with prettier + prettier-plugin-jsdoc
  • Make all exported fn params optional

I tried re-using the types from @types/node but these also seem incomplete/incorrect at places (e.g. SuiteFn) so instead based these changes on the README of this package.

Suggestions for further improvements:

  • Add describe.skip describe.todo it.skip it.skip
  • Open PR to improve @types/node and sync?

If these are welcome, happy to contribute another PR addressing those.

lib/test.d.ts Outdated Show resolved Hide resolved
lib/test.d.ts Outdated Show resolved Hide resolved
lib/test.d.ts Outdated Show resolved Hide resolved
@@ -29,40 +35,114 @@ interface TestOptions {
signal?: AbortSignal;
}

type TestFn = (t: TestContext) => any | Promise<any>;
type TestFn = (t: TestContext, done: (result?: any) => void) => any;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type TestFn = (t: TestContext, done: (result?: any) => void) => any;
type TestFn = <T>(t: TestContext, done: (result?: T) => void) => any | <T>(t: TestContext) => Promise<T>;

lib/test.d.ts Outdated
Comment on lines 50 to 53
export function test(
name: string,
options: TestOptions,
fn: TestFn
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export function test(
name: string,
options: TestOptions,
fn: TestFn
export function test<T>(
name: string,
options: TestOptions,
fn: TestFn<T>

lib/test.d.ts Outdated
Comment on lines 55 to 56
export function test(name: string, fn: TestFn): Promise<void>;
export function test(options: TestOptions, fn: TestFn): Promise<void>;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export function test(name: string, fn: TestFn): Promise<void>;
export function test(options: TestOptions, fn: TestFn): Promise<void>;
export function test<T>(name: string, fn: TestFn<T>): Promise<void>;
export function test<T>(options: TestOptions, fn: TestFn<T>): Promise<void>;

Using .prettierrc:
```json
{
  "plugins": ["prettier-plugin-jsdoc"]
}
```
@hongaar
Copy link
Author

hongaar commented Dec 16, 2022

@MoLow thanks for the review I can't find any reference for the fn return type signature of TestFn though?

Edit: misinterpreted your suggestion 🤦 will apply your changes!

Edit2: couldn't get this to work, tried:

type TestFn =
  | ((t: TestContext, done: (result?: any) => void) => any)
  | ((t: TestContext) => Promise<any>)

test(async (t) => {})

Which yields:

't' is declared but its value is never read. ts(6133)
Parameter 't' implicitly has an 'any' type, but a better type may be inferred from usage. ts(7044)

Making TestFn generic doesn't change much.

@hongaar hongaar requested a review from aduh95 December 16, 2022 09:32
lib/test.d.ts Outdated Show resolved Hide resolved
lib/test.d.ts Outdated Show resolved Hide resolved
lib/test.d.ts Outdated Show resolved Hide resolved
@hongaar
Copy link
Author

hongaar commented Dec 16, 2022

In order to test these changes, I wrote a quick tsd test in test/test.test-d.ts:

import { expectType } from "tsd";
import test, { describe, it } from "../lib/test.js";

// # test()

// Parameters
expectType<void>(await test());
expectType<void>(await test("name", { only: true }));
expectType<void>(await test("name", { only: true }, () => {}));
expectType<void>(await test({ only: true }));
expectType<void>(await test({ only: true }, () => {}));
expectType<void>(await test("name", () => {}));
expectType<void>(await test(() => {}));

// TestFn
expectType<Promise<void>>(test(() => {}));
expectType<void>(await test((t) => {}));
expectType<void>(await test(async (t) => {}));
expectType<void>(await test((t, done) => done()));
expectType<void>(await test(async (t, done) => done())); // Test will fail

// # describe()

// Parameters
expectType<void>(describe());
expectType<void>(describe("name", { only: true }));
expectType<void>(describe("name", { only: true }, () => {}));
expectType<void>(describe({ only: true }));
expectType<void>(describe({ only: true }, () => {}));
expectType<void>(describe("name", () => {}));
expectType<void>(describe(() => {}));

// # it()

// Parameters
expectType<void>(it());
expectType<void>(it("name", { only: true }));
expectType<void>(it("name", { only: true }, () => {}));
expectType<void>(it({ only: true }));
expectType<void>(it({ only: true }, () => {}));
expectType<void>(it("name", () => {}));
expectType<void>(it(() => {}));

In package.json:

{
  "scripts": {
    "test:types": "tsd --files test/test.test-d.ts"
  }
}

And run with:

npm i -D tsd
npm run test:types

If wanted, I will commit these changes (and add test:types to the test script) in a separate PR.

@hongaar hongaar requested a review from aduh95 December 16, 2022 11:11
@hongaar hongaar requested review from MoLow and removed request for aduh95 January 12, 2023 20:57
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.

3 participants