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

Run tsd in test script #162

Closed
wants to merge 3 commits into from
Closed

Run tsd in test script #162

wants to merge 3 commits into from

Conversation

mmkal
Copy link
Contributor

@mmkal mmkal commented Nov 28, 2020

I'm getting no errors locally, I think because of test not including tsd, and I don't see where it's run in GitHub Actions, so going to try out in a PR.

Leaving as a draft; feel free to ignore for now - if this goes red then all is well

I'm getting no errors locally, I think because of test not including `tsd`, and I don't see where it's run in GitHub Actions, so going to try out in a PR.
@mmkal mmkal marked this pull request as ready for review November 28, 2020 20:49
@mmkal
Copy link
Contributor Author

mmkal commented Nov 28, 2020

@sindresorhus this is an issue - the build above should not be green, unless there's something I'm missing! It looks like the call to tsd was commented out: https://github.com/sindresorhus/type-fest/blame/0337bcdebdad653915d35bf39428abf702ae5de5/package.json#L17-L18

I'm going to push a change to re-enable tsd. Locally I'm getting lots of failures.

CC @voxpelli

@mmkal
Copy link
Contributor Author

mmkal commented Nov 28, 2020

Update: ok, I turned tsd back on and there are a lot of errors, so I reverted the original bogus change. Most of them look like import errors, I'm guessing from types based on template literals being moved from source to ts41?

I tried fixing the imports but got a few other errors to do with types being declared locally but not exported - not sure if this is a tsd issue.

I'd be happy to help fix this, but it'd be good to get some guidance on how this is supposed to work. One possible way to fix that IMO would make this less "magical", by using expect-type (disclaimer - I maintain expect-type). Some advantages as I see them:

  • expect-type just exports a regular function so there's no extra process to get accidentally commented out.
  • similarly, it doesn't ship a patched version of typescript (2MB!)
  • it catches never and any types
  • tsd has special behaviour with expectError(...), so the test files have to be excluded because they don't actually compile.
  • expect-type uses expectTypeOf(...).not.toBeString() or similar, or // @ts-expect-error - which means the test files don't need to be excluded in tsconfig. If they compile with tsc, the tests are passing. If they don't, the tests are failing.
  • the test files being excluded mean that errors don't show up correctly in IDEs - the tsconfig.json, which includes "strict": true doesn't apply to them until tsd runs, so in the IDE options like strictNullChecks are off, and there are false negatives for things like expectType<string>(Math.random() < 0.5 ? 'foo' : null).
  • tsd assertions aren't super readable (again IMHO). Tests like this look weird to me - they have a redundant const declaration before the assertion, and it's easy to accidentally swap expected with actual. With expect-type it'd be expectTypeOf<CamelCase<'foo-bar'>>().toEqualTypeOf<'fooBar'>()

Let me know if you have a preference between fixing tsd issues, or you'd be open to taking a look at an expect-type port PR.

@mmkal mmkal mentioned this pull request Nov 28, 2020
5 tasks
@mmkal mmkal changed the title Test out CI process to see where errors show up Run tsd in test script Nov 28, 2020
@sindresorhus
Copy link
Owner

Yeah, I disabled tsd temporarily as there will be a new version in a few days that support TS 4.1. Didn't seem worth it to do any workarounds. I test locally before merging anyway.

@sindresorhus
Copy link
Owner

AFAIK, there are (or at least were) some reasons that tsd uses the TypeScript API instead of just pure types for checking. Maybe @SamVerschueren can chime in?

@voxpelli
Copy link
Collaborator

voxpelli commented Dec 1, 2020

There are two issues here:

  1. Getting tsconfig configured for all files
  2. Getting tsd-style checks working

1 can be achieved if we make it so that the tests on individual TS versions happens with a dedicated TS config.

@SamVerschueren
Copy link
Contributor

At first tsd was just pretty simple with generics and just used the TS compiler to verify that the file didn't have errors.

But requirements became more complex. We wanted to support expectError for instance, which can't be done unless you use @ts-expect-error probably nowadays.

The reason we use the TS API is also because we can detect if something is declared too wide. For instance if you do expectType<string | number>(add(1, 2)); it will fail indicating that string | number is declared too wide for number. So doing expectType<any>(add(1, 2)) will also fail unless add() really returns the any type. That's also the reason why we need a patched TS library, because those critical type-checking APIs aren't exported unfortunately. I looked at multiple packages who claimed to do enhanced type-checking but a lot of time failed for certain use cases, especially generics like Promise<T> or Observable<T>.

I hope this somewhat answers the question.

If everyone thinks the tsd API is unreadable we could definitely improve that I guess.

@voxpelli
Copy link
Collaborator

voxpelli commented Dec 1, 2020

@SamVerschueren Has there been attempts on getting upstream TS to export the needed API:s?

@SamVerschueren
Copy link
Contributor

@voxpelli I had in the past, see this PR microsoft/TypeScript#9943. It's a PR that got stale in 2016! They never did anything with it. You can read my replies at the bottom. There's an open issue to track this microsoft/TypeScript#9879, which is also open from 2016. They never respond...

sindresorhus added a commit that referenced this pull request Dec 3, 2020
@mmkal
Copy link
Contributor Author

mmkal commented Dec 3, 2020

@SamVerschueren thanks for the context! Re API readability, it might just be me, I find fluent-style assertions easier to reason about. I've been burned spending a lot of time trying to understand if a test was verifying the wrong thing when assertions look like expectEqual(a, b) - if they're different, is it a that's wrong or b? Whereas with expect(a).toEqual(b) it's clearer. Part of the problem is that tests are often protecting against future bugs, so when they're written a and b are equal anyway, and a different developer in the future is the one that sees the assertion failing.

Re patching TypeScript, it may not be necessary with conditionals anymore. I was able to write IsAny<T>, IsUnknown<T> and IsNever<T> helpers (maybe those kind of boolean types would be useful in this library):

expectTypeOf(1).toBeAny() // error
expectTypeOf(1).not.toBeAny() // ok

One very clear advantage of patching ts though is that error messages in CI are better, at least until something like microsoft/TypeScript#40468 can be merged enabling customised errors: mmkal/ts#152

@mmkal mmkal deleted the test-test branch December 3, 2020 20:51
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.

4 participants