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

Remove test import in README.md #65

Merged
merged 3 commits into from Mar 21, 2024
Merged

Remove test import in README.md #65

merged 3 commits into from Mar 21, 2024

Conversation

aryaemami59
Copy link
Collaborator

This PR:

@mrazauskas
Copy link
Contributor

Technically this is correct.

Did you notice that foo and bar are also not imported? Despite of that two assertions out of three are passing. They got type any. Simply imagine a situation where something anys because of refactoring code. expect-type is not able to catch such regression. (#64 is part of this problem.)

Another aspect: having Vitest import means that tsc will go on type checking Vitest types. Just put together a dummy repo with 2-3 test files and you will see the performance difference with import from Vitest and without. The difference is visible with naked eye. Having own test() helper would mean better performance for expect-type users.

@aryaemami59 aryaemami59 self-assigned this Mar 19, 2024
@aryaemami59 aryaemami59 added the documentation Improvements or additions to documentation label Mar 19, 2024
@mrazauskas
Copy link
Contributor

Another limitation of import {test} from 'vitest' is that test.only, test.skip and similar modifiers are defined on test(), but do not work as expected.

Recently I saw if() used as dummy test helper (that library is using tsd):

if ('when foo and bar are imported') {
  // make sure `foo` has type {a: number}
  expectTypeOf(foo).toMatchTypeOf<{a: number}>()

  // make sure `bar` is a function taking a string:
  expectTypeOf(bar).parameter(0).toBeString()
  expectTypeOf(bar).returns.not.toBeAny()
})

It has no .only or .skip and does not hurt performance.

README.md Outdated Show resolved Hide resolved
@mmkal
Copy link
Owner

mmkal commented Mar 19, 2024

The scope of this library is test assertions, not to be a test runner. So no need to define a test export.

The example in question here is just that... an example. It's to give readers an idea of what they need to do to test their types. foo and bar being undefined doesn't seem important to me. Your point that two of the three assertions pass anyway because of any is separate, but worth considering. I've gone back and forth on whether expect-type should be strict about any even on the non-strict types like toMatchTypeOf. Currently the answer is no but maybe we should change that.

As for performance, test.skip etc. Those are reasonable points and I think a good enough reason to remove the test wrapper from the first example. Nobody should install vitest just for the sake of using expect-type. They can just write plain assertions. And of course installing a full-blown test runner slows down tsc (although you should try again with skipLibCheck - IMO that's always worth enabling).

Having said all that, if we remove the test wrapper from the first example, it might be nice to have a "Usage with test frameworks" section with some recommendations.

@mrazauskas
Copy link
Contributor

Thanks for detailed explanation.

Within test frameworks section already exists at the bottom of the readme file. Extending it indeed sounds like a good idea.

@mrazauskas
Copy link
Contributor

mrazauskas commented Mar 19, 2024

I've gone back and forth on whether expect-type should be strict about any even on the non-strict types like toMatchTypeOf. Currently the answer is no but maybe we should change that.

Sorry about making noise here. What about documenting the above? This would explain / resolve #55 and likely #64 as well.

But documenting this behaviour will conflict with other parts of documentation, like: "first-class support for: any (as well as unknown and never)". Hm.. I understood that anys are always explicitly handled, pointers to tsd issues made me think so.

Also "make sure" in the following is misguiding, because foo can be also any:

expect-type/README.md

Lines 15 to 16 in 9cda9e0

// make sure `foo` has type {a: number}
expectTypeOf(foo).toMatchTypeOf<{a: number}>()

@mmkal
Copy link
Owner

mmkal commented Mar 19, 2024

Yes, some clarification needed, how it was intended:

toEqualTypeOf: protects against any, lets you assert an exact type
toMatchTypeOf: ensure that a value matches a type, or since typescript uses structural typing, "has" a type. That's why any passes for toMatchTypeOf.

In practice, though, you're right that very few people will want an any except in a very small number of cases. And in those cases they could use toBeAny(). And now would be the time to make that change (pre-v1).

@mmkal mmkal mentioned this pull request Mar 19, 2024
@mmkal
Copy link
Owner

mmkal commented Mar 19, 2024

Update: I thought a bit more, and I am struggling to think of any good/practical use for having assertions like expectTypeOf(foo).toMatchTypeOf<{a: 1}>() pass, when foo has type any. So, I opened #66 to make it so the only thing you can do with an any is expectTypeOf(foo).toBeAny().

FYI @mrazauskas

@mrazauskas
Copy link
Contributor

mrazauskas commented Mar 19, 2024

As for performance, test.skip etc. Those are reasonable points and I think a good enough reason to remove the test wrapper from the first example. Nobody should install vitest just for the sake of using expect-type. They can just write plain assertions. And of course installing a full-blown test runner slows down tsc (although you should try again with skipLibCheck - IMO that's always worth enabling).

I used expect-type repo to make some performance tests with hyperfine. tsc was running using current tsconfig.json where only include was changed to: "include": ["test/types.test.ts", "test/usage.test.ts"] and "types": [] added.

With import {test} from 'vitest':

  • 1056 ms (average of 10 runs, with 2 warm up runs)

Replacing the import with declare function test(name: string, fn: () => void): void;:

  • 865 ms (average of 10 runs, with 2 warm up runs)

Yep, "skipLibCheck": true is set, but it does not help.

@aryaemami59 aryaemami59 changed the title Fix missing test import in README.md Remove test import in README.md Mar 21, 2024
@aryaemami59
Copy link
Collaborator Author

@mmkal I removed the test import altogether.

@mmkal mmkal merged commit 9015dcd into main Mar 21, 2024
2 checks passed
@mmkal mmkal deleted the fix-test-import-in-readme branch March 21, 2024 20:44
kodiakhq bot pushed a commit to X-oss-byte/Nextjs that referenced this pull request Apr 1, 2024
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [expect-type](https://togithub.com/mmkal/expect-type) | [`0.18.0` -> `0.19.0`](https://renovatebot.com/diffs/npm/expect-type/0.18.0/0.19.0) | [![age](https://developer.mend.io/api/mc/badges/age/npm/expect-type/0.19.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/expect-type/0.19.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/expect-type/0.18.0/0.19.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/expect-type/0.18.0/0.19.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>mmkal/expect-type (expect-type)</summary>

### [`v0.19.0`](https://togithub.com/mmkal/expect-type/releases/tag/0.19.0)

[Compare Source](https://togithub.com/mmkal/expect-type/compare/0.18.0...0.19.0)

#### What's Changed

-   Fix `.omit()` to work similarly to `Omit` by [@&#8203;aryaemami59](https://togithub.com/aryaemami59) in [mmkal/expect-type#54
-   Add JSDocs to everything by [@&#8203;aryaemami59](https://togithub.com/aryaemami59) in [mmkal/expect-type#56
-   Remove `test` import in `README.md` by [@&#8203;aryaemami59](https://togithub.com/aryaemami59) in [mmkal/expect-type#65

**Full Changelog**: mmkal/expect-type@0.18.0...0.19.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/X-oss-byte/Nextjs).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add test() helper
3 participants