Deprecation expectations

[cartant]

In RxJS, we have a significant number of overload signatures that are deprecated and seemingly unrelated changes to overload signatures and effected false positives for deprecations.

It would be nice to be able to add tests to ensure that deprecations as behaving as they should. To this end, I've added a TSLint rule that can be used with dtslint to test our deprecations.

The tests will look something like this:

it('should deprecate correctly', () => {
  of(queueScheduler); // $ExpectDeprecation
  of(a, queueScheduler); // $ExpectDeprecation
  of(a, b, queueScheduler); // $ExpectDeprecation
  of(a, b, c, queueScheduler); // $ExpectDeprecation
  of(a, b, c, d, queueScheduler); // $ExpectDeprecation
  of(a, b, c, d, e, queueScheduler); // $ExpectDeprecation
  of(a, b, c, d, e, f, queueScheduler); // $ExpectDeprecation
  of(a, b, c, d, e, f, g, queueScheduler); // $ExpectDeprecation
  of(a, b, c, d, e, f, g, h, queueScheduler); // $ExpectDeprecation
  of(a, b, c, d, e, f, g, h, i, queueScheduler); // $ExpectDeprecation  
  of<A>(); // $ExpectDeprecation
  of(); // $ExpectNoDeprecation
  of(a); // $ExpectNoDeprecation
  of(a, b); // $ExpectNoDeprecation
  of(a, b, c); // $ExpectNoDeprecation
  of(a, b, c, d); // $ExpectNoDeprecation
});

Would you be open to adding two additional expectations to this package so that the presence and absence of deprecations could be tested?

[sindresorhus]

Makes sense to me as long as we clearly document to not use it for unambiguous situations (no overloads, for example).

[SamVerschueren]

I already have some code locally to cover this because I experimented how feasible this would be to implement. It turned out to be pretty straight forward. Only have to finish it and then it should be ready.

@sindresorhus Can you clarify the overload thing? The of in the example of @cartant is an overload where it makes sense I guess to have it.

[SamVerschueren]

I implemented it locally, but was wondering if we should do this for properties as well.

interface Unicorn {
	/**
	 * @deprecated
	 */
	readonly property: string;
}

const unicorn: Unicorn = {
	property: 'unicorn'
};

expectDeprecated(unicorn.property);

Should it check this as well? Or should we currently only allow CallExpression nodes?

[SamVerschueren]

In the TS AST this is called a PropertyAccessExpression. I can't find a way to retrieve the JSDoc tags from the signature though 🤔. Probably missing something...

If we want to do this, are there other things that might be marked as deprecated? An entire interface for instance?

/**
 * @deprecated
 */
interface Unicorn {
	readonly property: string;
}

Not sure if this is possible. Might be opening pandora's box here 😂. I would be fine to start with CallExpression for now and see if there are use cases for others. What do you guys think?

[cartant]

FWIW, I cheated and lifted TSLint's deprecation-sniffing implementation:

https://github.com/cartant/tslint-etc/blob/b184b205127a1ec2e3d25cb6a76e0d229996d8a0/source/rules/expectDeprecationRule.ts#L46-L48

https://github.com/cartant/tslint-etc/blob/b184b205127a1ec2e3d25cb6a76e0d229996d8a0/source/support/tslint-deprecation.ts

Which, IIRC, finds deprecations for properties.

[sindresorhus]

@sindresorhus Can you clarify the overload thing? The of in the example of @cartant is an overload where it makes sense I guess to have it.

I just want to make sure users don't create moot assertions, like #51 (comment), where nothing else could affect it, so there's no reason to test whether it's deprecated.

Should it check this as well?

Yes, we should check properties too.

[SamVerschueren]

This is what I have right now as a test for expectDeprecated

import {expectDeprecated} from '../../../..';
import concat, {Unicorn, Options} from '.';

// Methods
expectDeprecated(concat('foo', 'bar'));
expectDeprecated(concat(1, 2));
//=> Expected `(foo: number, bar: number): number` to be marked as `@deprecated`

// Properties
const options: Options = {
	separator: ',',
	delimiter: '/'
};

expectDeprecated(options.separator);
expectDeprecated(options.delimiter);
//=> Expected `Options.delimiter` to be marked as `@deprecated`

// ENUM
expectDeprecated(Unicorn.UNICORN);
expectDeprecated(Unicorn.RAINBOW);
//=> Expected `Unicorn.RAINBOW` to be marked as `@deprecated`

Works the same for expectNotDeprecated. Have to wrap up the docs and then I believe this is good to go? Or do I still miss a use case?

I was in doubt if you should be able to validate the deprecation message.

const enum Unicorn {
	/**
	 * @deprecated - Use Rainbow instead
	 */
	UNICORN = '🦄',
	RAINBOW = '🌈'
}

expectDeprecated(Unicorn.UNICORN, 'Use Rainbow instead');

But decided not to (for now) as I wasn't sure if it was useful.

[cartant]

I don't know what the definition for concat is and I'm not sure how to interpret what you've described as a test, but IMO the most important uses cases are functions and method overloads in which some signatures are deprecated and others are not. E.g.

https://github.com/cartant/tslint-etc/blob/0571844378fa00703d181943db9b040224afb20d/fixtures/expect-deprecation/default/fixture.ts.lint#L6-L9

And for methods, too. (Which I didn't test, 'cause I just lifted the TSLint deprecation sniffer.)

[SamVerschueren]

Sorry, didn't provide the concat definition. It's an overload

declare const concat: {
	/**
	 * @deprecated
	 */
	(foo: string, bar: string): string;
	(foo: number, bar: number): number;
};

export default concat;

Thanks for linking your tests. I was looking for them but couldn't find them.