Rxjs: dtslint effects yet another error

[cartant]

Bug Report

I've about had enough of dtslint. What was working, this morning, on the notebook I use at home is now not working on the notebook I have at the office - despite dtslint being pinned to a specific version.

It's effecting this error:

Error: ENOENT: no such file or directory, mkdir '/Users/nicholasjamieson/.dts/perf'

I'm inclined to abandon it in favour of a more straightforward lint-based solution.

I don't relish reinventing the wheel, but this wheel keeps falling off.

[cartant]

There's now an alternative in tsd, but switching to it would involve re-writing a tonne of tests.

[Georift]

Hi @cartant, I've had a first crack at converting the tests over to the tsd library you mentioned. Certainly feels like a much nicer syntax than the funky comment assertions from tsdlint.

Would you mind taking a look and letting me know if I'm on the right track? If so I'd be happy to spend some time working through the rest of the tests. :)

[cartant]

@Georift Thanks. Looks good.

I was able to get dtslint to work on this MacBook by creating a /Users/nicholasjamieson/.dts/perf directory, but my opinion of dtslint is unchanged. I think we should move away from it.

You might want to hold off, before you commit too much time to the conversion. Whether or not all of the tests should be converted is something that would have to be discussed, first.
The pros and cons will need to be weighed up - for that, this issue might be helpful.

Also, any conversion would have to be part of some process - I would not want to review a massive PR that converted everything. And the conversion would also need to refactor the tests to simplify them - I'd like to use helpers like these to reduce the boilerplate and increase the coverage (by not testing with just strings and numbers, etc.) And I'm not going to have to time to get involved in this until around mid-September.

[cartant]

One concern with tsd is the fact that this PR has been sitting for some time without being discussed or merged. It's not encouraging.

https://github.com/SamVerschueren/tsd/pull/21

[Georift]

@cartant I also noticed all that was required was creating that in my home folder, it seems to be recording metrics about the memory usage of the linter which can be used by other packages. Unfortunately there is no mechanism to disable this.

Regarding the pros & cons I'm happy to try and summarise however I'd want someone to take a second pass as I'm not as proficient with TS as JS and mightn't do it justice. Shall I do this? Or is there anything else I can do to help at this point prior to you being availiable mid-September?

[cartant]

Regarding the pros & cons I'm happy to try and summarise.

That would be appreciated.

... is there anything else I can do to help at this point ...

I think refactoring the tests to use helpers - like those mentioned above - is a higher priority than switching to something other than dtslint. It's quite likely that - should we decide to switch to something else - the switching process could be automated.

I'll try to write up some guidelines for what I'd like to see happen with the test and will put them into an issue. I have some notes, somewhere, that I made a few months ago. If you have a look at the way the helpers are used in https://github.com/ReactiveX/rxjs/pull/4474, it will give you some idea of what I'm talking about.

[cartant]

Related: https://github.com/ReactiveX/rxjs/pull/5027#issuecomment-533939349

[SamVerschueren]

I just released [email protected] which now supports strict type assertions https://github.com/SamVerschueren/tsd#strict-type-assertions.

The main problem with dtslint type assertions is that order matters. string | number is not identical to number | string.

It took me a while to figure this out, that's why the initial PR got stale because I didn't want to go for string matching. So now we are actually comparing the types on type level.

I have a test here which shows what I mean

expectType<Observable<string | number> | Observable<Date> | Observable<Symbol>>(
    one<Observable<Date> | Observable<Symbol> | Observable<number | string>>()
);

one<T>() is just a function which returns type T

This assertion succeeds, which it does not in dtslint because of the order.

Feel free to play with it and let me know if you find issues or have ideas to improve tsd.

[cartant]

@samccone Thanks. I'll check it out.

I noticed this comment. When you say that expectType is strict, does that mean it and expectError behave like this:

expectType<string | number>(add(1, 2)); // error, as expectation is too wide
expectError<string | number>(add(1, 2)); // no error

I'd be happy if that's the case.

[SamVerschueren]

@cartant yes for expectType, no for expectError. I wasn't sure about expectError so I asked for feedback about its behaviour https://github.com/SamVerschueren/tsd/issues/10#issuecomment-535686968. So thanks for asking, because it's valuable feedback if that's your first question about it :).

[cartant]

@SamVerschueren Regarding tsd, is it in any way extensible? One of the things I've contemplated is creating a lint rule - perhaps for use with dtslint - that determines whether or not a matched function signature is deprecated. If tsd isn't extensible, would an expectation like this be something you would welcome? The next couple of RxJS versions are going to involve working with and managing deprecated signatures and I'd like to avoid the accidental deprecations that have occurred in the past.

[SamVerschueren]

@cartant No it's not extensible at this point. We added the things that are important to us but are definitely open to see what people need when they want to test their type definitions.
Could you open a new issue in tsd directly so we can discuss things? Examples regarding the deprecation signatures and how you think these should be validated would be nice.

We can also look in exposing some kind of plugin system. I will think about how it could look, but I would like to see first if the deprecation checks should be part of tsd itself. Which I feel is perfectly possible to add.

[cartant]

Closing this, as it's resolved by #5123. I'll create another issue, at a later date, to look at switching to tsd - via a code mod - but, ATM, getting the TypeScript signatures sorted is a higher priority.