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

Deprecated attribute #390

Open
DickvdBrink opened this Issue Aug 7, 2014 · 19 comments

Comments

Projects
None yet
@DickvdBrink
Copy link
Contributor

DickvdBrink commented Aug 7, 2014

It would be cool to annotate a method or property with a deprecated attribute

Proposal

If a warning could be issued when using a deprecated method or property it is easier to upgrade to a newer library version (only when the definitions are up to date of course)

Syntax: Same as JSDoc, a simple comment

/**
 * @deprecated Use other method
 */
public foo() {
}

If supported in .d.ts files, this would really help with upgrading to a newer version.

@RyanCavanaugh

This comment has been minimized.

Copy link
Member

RyanCavanaugh commented Aug 7, 2014

How would this flow through the type system?

interface X {
    /*
     * @deprecated Use DoOtherThing instead
     */
    doThing(): void;
}

interface Y {
    /*
     * Still supported
     */
    doThing(): void;
}

class Z implements X, Y {
    doThing() {}
}

var g = new Z();
g.doThing(); // OK? Not OK?

Presumably you'd be able to mark a single overload (but not others) as deprecated; would referencing (but not calling) a function with a deprecated overload be an error?

@DickvdBrink

This comment has been minimized.

Copy link
Contributor Author

DickvdBrink commented Aug 7, 2014

Hmz, tricky question.. didn't thought about this one actually.

Maybe it is only an error in this case when calling it like this:

var x: X = new Z();
x.doThing() // it selected to @deprecated methode so error

var y: Y = new Z();
x.doThing() // still supporter, no error

Not really sure though, feels a bit error prone...

@JustinBeckwith

This comment has been minimized.

Copy link

JustinBeckwith commented Jan 6, 2015

C# will allow what you've written above, because it does not react to [Obsolete] attributes on interfaces - only base class implementations. Given the lack of multiple inheritence, this isn't really an issue. Here's how it's handled in C#:

class Program
    {
        static void Main(string[] args)
        {
            var x = new Zoo();

            // doThing is allowed, even though it is obsolete on interface X
            x.doThing();

            // doStuff is not allowed. It is not obsolete on the interface, but is obsolete on the base class. 
            x.doStuff();
            Console.Read();

        }
    }

    interface X
    {
        /// <summary>
        /// 
        /// </summary>
        [Obsolete("boo", true)]
        void doThing();
    }

    interface Y
    {
        void doThing();
        void doStuff();
    }

    class Zoo : ZooBase, X
    {
        public void doThing()
        {
            Console.WriteLine("boo");
        }

        [Obsolete]
        public override void doStuff()
        {
            Console.WriteLine("boo");
        }
    }

    abstract class ZooBase
    {
        [Obsolete("boo", true)]
        public abstract void doStuff();
    }
@mhegazy

This comment has been minimized.

Copy link
Contributor

mhegazy commented Jan 12, 2015

I think this is fair. the class provided a re-declaration of the method. if you were to use the interface directly you would get an error:

var g = new Z();
g.doThing(); // OK, this is the class definition of doThing()

var e: X = new Z();
e.doThing(); // Error, X.doThing() is deprecated.

Now consider this:

interface Person {
    name: string;

    /** @deprecated use birthDate instead */
    age?: number;

    birthDate?: Date;
}

declare function doStuff(p: Person): void;

// should calling doStuff with age instead of birthDate be an error:

doStuff({
    name: "Joe",
    age: 25          // Error?: Person.age is depreciated
});

if the answer is yes, then the above example should be an error on the extends clause, prohibiting the implementation of a depreciated method on the interface.

it also means you can not depreciate a non-optional member of an interface, cause how else would you use it.

@louy

This comment has been minimized.

Copy link

louy commented Nov 27, 2015

I believe this shouldn't cause an error but rather a warning.
For the situation @RyanCavanaugh mentioned, a function would only be deprecated if there doesn't exist any non-deprecated implementation.
As for implementing an interface-deprecated method in a class, you should get a warning once in the class implementation, not each time you're calling the method.

interface IX {
  @deprecated
  function doSomething(): void;
}

class X implements IX {
  function doSomething(): void; // Warning: IX.doSomething is deprecated

  @deprecated
  function doSomethingElse(): void;
}

const x = new X();
x.doSomething(); // Ok...
x.doSomethingElse(); // Warning: X.doSomethingElse is deprecated
@kitsonk

This comment has been minimized.

Copy link
Contributor

kitsonk commented Nov 27, 2015

That syntax collides with decorators. 😦

Considering this hasn't seen any action in a long time, and now that the compiler parses and integrates JSDoc, it could in theory understand and warn on /* @depecrated */ JSDoc, but is there yet the concept of warning in TypeScript or does it still consider everything an error?

Otherwise this should be the domain of something like tslint @adidahiya, thoughts?

@louy

This comment has been minimized.

Copy link

louy commented Nov 27, 2015

Yeah I know. I didn't know that the complier considered JSDoc comments. I thought we might have a reserved keyword or something.
I also think this is beyond the scope of tslint. Linting should be about coding style, not type checking.

AFAIK TS has no concept of warnings. In my opinion however, it should. Similar to ESLint's error/warning concepts.

@kitsonk

This comment has been minimized.

Copy link
Contributor

kitsonk commented Nov 27, 2015

Linting should be about coding style, not type checking.

I disagree... It should be about things deal with code quality, things that are syntactical errors. tslint and other linting tools do lots of quality checking, like unused variables, conditional assignments, switch statement drop through and defaults, etc.

In fact we draw the defenition from the UNIX lint tool which:

In computer programming, lint is a Unix utility that flags some suspicious and non-portable constructs (likely to be bugs) in C language source code; generically, lint or a linter is any tool that flags suspicious usage in software written in any computer language.

@louy

This comment has been minimized.

Copy link

louy commented Nov 27, 2015

@kitsonk okay I'm convinced. I'll open a new issue in tslint's repo then.

@felixfbecker

This comment has been minimized.

Copy link

felixfbecker commented Mar 27, 2016

I also think this is something that does not need a language feature, but belongs to documentation (jsdoc) and can be checked by something like tslint.

LOZORD added a commit to LOZORD/DefinitelyTyped that referenced this issue Aug 20, 2016

Fixes #10684
I kept the deprecated `pick` declarations, but added the proposed (JSDoc-influenced) [deprecation annotation](Microsoft/TypeScript#390) to them. The added `pickone` and `pickset` declarations have identical types to their respective `pick` flavors.

@LOZORD LOZORD referenced this issue Aug 21, 2016

Merged

Fixes #10684 #10751

0 of 3 tasks complete

vvakame added a commit to DefinitelyTyped/DefinitelyTyped that referenced this issue Sep 14, 2016

Fixes #10684 (#10751)
I kept the deprecated `pick` declarations, but added the proposed (JSDoc-influenced) [deprecation annotation](Microsoft/TypeScript#390) to them. The added `pickone` and `pickset` declarations have identical types to their respective `pick` flavors.

vvakame added a commit to DefinitelyTyped/DefinitelyTyped that referenced this issue Oct 25, 2016

Added Seeded interface to Chance definition (#11919)
* Fixes #10684

I kept the deprecated `pick` declarations, but added the proposed (JSDoc-influenced) [deprecation annotation](Microsoft/TypeScript#390) to them. The added `pickone` and `pickset` declarations have identical types to their respective `pick` flavors.

* Added Seeded interface to Chance definition
@PanayotCankov

This comment has been minimized.

Copy link

PanayotCankov commented Dec 30, 2016

We have homegrown @deprecated decorator but adding this in the language feature so it can be better visible in .d.ts-es would be great!

Can you make two flavors of it (like @deprecated and @obsolete) where one will warn and the other will err. The workflow would be to make stuff that is about to be removed with @deprecated for a version or two so the downstream dependencies can have time to upgrade and then move from @deprecated to @obsolete in the .d.ts when the member is deleted. At runtime it would be up to the library to promise the @deprecated to work even though with degraded quality, while the @obsolete will be expected to throw.

@adamvoss

This comment has been minimized.

Copy link

adamvoss commented Jul 1, 2017

@PanayotCankov The distinction you make between @deprecated and @obsolete is not intuitive to me, though maybe it is or other people. Perhaps @removed would communicate the expected behavior? I am not sure I've ever found myself needing multiple levels of deprecation, but it could still be a good idea.

@carltoncolter

This comment has been minimized.

Copy link

carltoncolter commented Nov 22, 2017

Where are we with this issue? @deprecated on an interface doesn't cause any kind of warning in Visual Studio and it should... I'm not talking about layers deep. Can we at least get one layer? So if we are trying to use something that has the @deprecated tag on it, it shows a warning or something in the IDE?

The @deprecated intellisense isn't working in visual studio.... is it working somewhere else?

@felixfbecker

This comment has been minimized.

@ikokostya

This comment has been minimized.

Copy link

ikokostya commented Nov 22, 2017

@felixfbecker The main problem with this solution is it doesn't check external modules. If typescript adds support for this it will work with external modules (and type declarations).

@carltoncolter

This comment has been minimized.

Copy link

carltoncolter commented Nov 22, 2017

Thanks @felixfbecker ! I figured out why I couldn't get tslint to work right. My problemmatcher wasn't working. I had to setup my own in my lint task...

{
    "version":"2.0.0",
    "tasks": [
        {
            "type":"npm",
            "script": "lint",
            "group": {
                "kind": "build",
                "isDefault": true
            },
            "problemMatcher": {
                "owner":"tslint",
                "severity": "warning",
                "fileLocation": ["absolute"],
                "pattern":[
                    {
                        "regexp": "^(WARNING|ERROR):(\\s+\\(\\S*\\))?\\s+(\\S.*)\\[(\\d+), (\\d+)\\]:\\s+(.*)$",
                        "severity": 1,
                        "file": 3,
                        "line": 4,
                        "column": 5,
                        "message": 6
                    }
                ]
            }
        }
    ]
}

I'm not sure why the regular pattern matcher base $tslint5 wasn't picking up the errors. Now it is...

@yawaramin

This comment has been minimized.

Copy link

yawaramin commented Dec 26, 2017

IMO this needs to be provided by tsc, using the /** @deprecated */ JSDoc. Here's why. Evolving a statically-typed codebase is a critical part of the job. Deprecating certain items in the code is the best way to do that while a project is in production. The lack of proper, granular, deprecation holds projects back from evolving their codebase towards safer types.

Let me try to answer @RyanCavanaugh's question ( #390 (comment) ) from a type-theoretic perspective. When class Z implements interfaces X and Y, it is essentially an intersection type between X and Y, hence in Z the doThing method is considered to be a member of both X and Y. Hence, it is trivially a member of X, so it should trigger the deprecation error.

@madluxit

This comment has been minimized.

Copy link

madluxit commented Jan 9, 2018

That's correct. As SDK developer, I cannot force all the potential users of my API to use tslint in order to spot warnings that should be spotted at compile time (in addition obviously to IDE inline support).
tslint could be used to improve the quality of the user code style, and expecting that customers are able to spot API misuse issues only through that is oversimplification.

@RikkiGibson

This comment has been minimized.

Copy link

RikkiGibson commented May 26, 2018

@madluxit has got it. @deprecated is most useful as a feature for library developers who need to convey changes to their users. If we are only able to provide deprecation warnings to people who have tslint configured to see those warnings, we might as well console.warn() on the first call to that function or something instead.

(This does leave out the fact that lots of usage is bound to be direct from JS, or from another compile-to-JS language--so who knows what a really decent story for informing users of deprecation is besides good release notes and docs.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment