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

Comparison to dtslint #10

Open
eps1lon opened this issue Oct 15, 2018 · 19 comments
Open

Comparison to dtslint #10

eps1lon opened this issue Oct 15, 2018 · 19 comments

Comments

@eps1lon
Copy link

eps1lon commented Oct 15, 2018

I'd like to know how your library compares to dtslint. As of right now it looks like I can achieve the same with dtslint's expect rule and tslint.

Do you have any issues with dtslint that you want to fix or features that you want to add?

@BendingBender
Copy link
Collaborator

BendingBender commented Mar 1, 2019

From my experience with dtslint vs tsd:

  • it's not possible to make an unnoticed typo in expectType<...>(...) (quite possible with dtslint: $ExectType, ...) to render the assertion meaningless
  • the asserted types don't have to be absolutely identical to the types TS derives internally (in dtslint: string | number will pass, string|number not, string[] will pass, Array<string> not)
  • each tested against type must have been exported, an assertion for a non-exported type won't pass, this way one can never forget to export a type
  • checks for typings and files fields in package.json, points to broken settings/paths
  • quite easy to set up, no config needed

@BendingBender
Copy link
Collaborator

BendingBender commented Mar 1, 2019

This module is, however, quite young, it still misses a few features:

  • no equivalent for $ExpectError
  • tests for assignability, not type equality:
expectType<{[key: string]: any}>({foo: 'bar'}); // => OK
// while {[key: string]: any} is definitely not the same as {foo: 'bar'}
expectType<string | number>(1); // => OK

@sindresorhus
Copy link
Collaborator

sindresorhus commented Mar 2, 2019

no equivalent for $ExpectError

I really wish we had this one.

not possible to tweak compiler options (strict would be very useful)

I assumed it was already strict. If not, we should definitely change the default to strict and add a way to opt-out.

@BendingBender
Copy link
Collaborator

BendingBender commented Mar 2, 2019

I assumed it was already strict. If not, we should definitely change the default to strict and add a way to opt-out.

See: #13. :-)

It is not enabled by default, but we can discuss this issue there.

@blakeembrey
Copy link

blakeembrey commented Mar 16, 2019

@sindresorhus I was thinking about $ExpectError and I think you can do it using conditional types and extracting the types you wish to validate. Here's an example:

export type TypeOf<T, U> = Exclude<U, T> extends never ? true : false;

export function test(_: string): void {}

expectType<TypeOf<[string], Parameters<typeof test>>>(true);
expectType<TypeOf<[number], Parameters<typeof test>>>(false);

@SamVerschueren
Copy link
Collaborator

expectError is now also part of the API

expectError<string>(5);
//=> Passes

expectError<string>('🦄');
//=> Expected an error, but found none.

expectError(foo(1, 2));
//=> Passes if `foo` only accepts strings as argument types

@SamVerschueren
Copy link
Collaborator

SamVerschueren commented Mar 16, 2019

Are there any other things that don't work for you? Please feel free to let me know so I (or we) can fix this.

@BendingBender What do you exactly mean with:

expectType<{[key: string]: any}>({foo: 'bar'}); // => OK
// while {[key: string]: any} is definitely not the same as {foo: 'bar'}

Why aren't these the same? foo is a key of type string and any matches everything, so also the string bar. I probably don't see the issue so feel free to elaborate :).

@eps1lon
Copy link
Author

eps1lon commented Mar 16, 2019

Are there any other things that don't work for you?

I'm missing some context why we need this package. Does your expectError API provide some additional functionality compared to $ExpectError? Same for expectType.

@SamVerschueren
Copy link
Collaborator

It certainly does, just read this issue microsoft/dtslint#191

// dtslint
const o = from([of(1), ['test'], iterable]); // $ExpectType Observable<IterableIterator<number> | Observable<number> | string[]>

// tsd-check
expectType<Observable<IterableIterator<number> | Observable<number> | string[]>>(from([of(1), ['test'], iterable]));

Already explained by @BendingBender, you can't make typo's because the TypeScript compiler will tell you it's wrong. dtslint uses comments, how many typo's do people make when writing those comments? Especially with those complex types.

Secondly, tsd-check uses the REAL types. What I mean is, if I expect an RxJS observable, but the function is returning a Zen-observable, tsd-check will catch this, I don't think dtslint does because it uses string matching and for them, 'Observable' === 'Observable'.

And thirdly, one I find quite nice to have, is that when writing tests, the TS compiler already gives you feedback if you do something wrong.

expectType<string>(1);

This will already be marked with a red line by your text editor because the compiler detects a type mismatch. This is not the case with dtslint either.

So yes, in my opinion, this library has benefits over dtslint. But maybe I'm biased as I'm the author ;).

@eps1lon
Copy link
Author

eps1lon commented Mar 16, 2019

Just to be clear I didn't mean to attack you. I think all of those points are perfectly valid. I just think it's very important to list related work in packages and how it compares. Otherwise the choice for or against a package becomes arbitrary.

@SamVerschueren
Copy link
Collaborator

Do you think it would help if we documented in the readme?

@SamVerschueren
Copy link
Collaborator

If we document this, I don't want it to become oh look, ours is much better because this and this reason. Not sure how to do it decently without attacking anyone's work.

@BendingBender
Copy link
Collaborator

BendingBender commented Mar 17, 2019

@SamVerschueren

Why aren't these the same? foo is a key of type string and any matches everything, so also the string bar. I probably don't see the issue so feel free to elaborate :).

What I mean is that the type {[key: string]: any} will accept pretty much any object and thus the whole assertion is rendered useless. This is an extreme case, in reality it can be much more subtle:

const foo = () => 'foo';

expectType<string | undefined>(foo());

This will pass. The problem is that actually I wanted to assert that foo returns a string | undefined and my expectation was that this check should fail. But string is assignable to string | undefined and thus this passes. This is what I mean when I say that this is not actually a type comparison but an assignability check.

@mike-north
Copy link

I'm looking to try this library out, and can't figure out if there's good support for the "sufficiently narrow" test cases I typically write when working with dtslint.

I usually rely on this aspect of dtslint that @BendingBender pointed out out

[in tsd-check] the asserted types don't have to be absolutely identical to the types TS derives internally (in dtslint: string | number will pass, string|number not, string[] will pass, Array not)

i.e., if I have a function foo() that returns a Promise<[number, number]> I could write

foo(); // $ExpectType Promise<[number, number]>

and I'd be good to go, since I'm asserting against the stringified type return value of foo().

In this library I could do

expectType<Promise<[number, number]>>(foo());

But it also would pass if I introduced a bug causing foo to return a wider type, since structural type equivalence is the means of assertion, as opposed to exact match of stringified types

As a result, the following (potentially undesirable) changes to foo would also pass the assertion above

function foo(): Promise<[number, any]>; 
function foo(): Promise<[any, any]>;
function foo(): Promise<any>;
function foo(): any;  

Are there plans to handle any cases like these? Do consumers of this library typically just worry about testing that types are sufficiently wide and don't care as much about them being sufficiently narrow?

@BendingBender
Copy link
Collaborator

@mike-north Just made a PR to improve type assertions: #21.

@SamVerschueren
Copy link
Collaborator

Strict type assertions will be available somewhere this weekend. Have to wrap little things up and then it's good to go.

The benefit in tsd versus dtslint is that dtslint uses the string representation of the type. This means that string | number is not identical to number | string. This is pretty annoying, especially for complex types. RxJS had lots of issues with this.

Tsd checks assignability on type level, which means that we don't have that problem 🎉 . Keeping you guys posted when I release this feature.

@SamVerschueren
Copy link
Collaborator

I just released tsd@0.9.0 with support for strict expectType assertion 🎉.

@BendingBender I wasn't sure what should happen in case of expectError. Any feedback would be more than welcome.

expectError<string | number>(add(1, 2));`

add returns number and currently the tests will fail with Expected an error, but found none. because number is assignable to string | number. But should the test succeed because the type string | number is too wide? 🤔

I wasn't sure about this and like some feedback on this topic.

@danvk
Copy link

danvk commented Oct 10, 2019

A few more points to contribute to this comparison:

  • dtslint isn't fooled by any types, while let v: any; expectType<T>(v); will pass for all types T. This is because dtslint does string comparisons on the types, rather than checking for structural relationships. This is also why dtslint draws meaningless distinctions like A | B vs. B | A vs A|B. See Check for any types in strict expectType #43.

  • In a similar vein, if you care that your function returns a named type like Vector2D rather than a type like { x: number; y: number; } then you can check that with dtslint. These are structurally identical, so tsd can't distinguish them. This may or may not be something you want.

  • dtslint lets you assert types in places where a function call isn't permitted, e.g. on inferred parameter types in callbacks:

map(['a', 'b', 'c'], (
  x,  // $ExpectType string
  i,  // $ExpectType number
  xs  // $ExpectType string[]
) => 0);

To do the same thing with tsd you need to add a function body:

map(['a', 'b', 'c'], (x, i, xs) => {
  expectType<string>(x);
  expectType<number>(i);
  expectType<string[]>(xs);
});
  • dtslint is designed to run your declarations through multiple versions of TypeScript. If you care about supporting older versions, or figuring out exactly which version of TypeScript your declarations require, then this may be helpful.

  • With "strict" checking, expectType looks like an ordinary function call with a generic type parameter but does not behave like one. If you understand structural typing and realize that expectType is treated differently by tsd and tsc, then good on you. But if not, this may be a source of confusion and a stumbling block in building an intuition for structural typing and assignability relationships.

@BendingBender makes a great point about misspelled $ExpctType assertions silently passing. That is scary!

Getting feedback from tsc in your editor is nice. Since dtslint is built on tslint, there's no reason you couldn't get it to do the same thing. But I'm not aware of anyone doing that. At least to me, a linter does feel like the right tool to do this since it's a bit different than ordinary TypeScript type checking (two-way assignability checks for tsd, comparison of types as strings for dtsint). There's no reason you couldn't build an editor plugin for tsd, either.

The point about 'Observable' === 'Observable' even if they're not the same Observable type is also a good one. In general dtslint is a more conservative tool, but this is the only situation I'm aware of where tsd would rightfully complain while dtslint would not.

My 2¢!

@SamVerschueren
Copy link
Collaborator

Thanks for sharing this @danvk!

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

No branches or pull requests

7 participants