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

Quick info shouldn't default to showing jsdoc from first overload #45337

Open
SpeedoPasanen opened this issue Aug 5, 2021 · 7 comments
Open
Assignees
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript

Comments

@SpeedoPasanen
Copy link

SpeedoPasanen commented Aug 5, 2021

#43053

This is still happening at least with the tap function from rxjs. VSCode 1.58.2. TS version in project: 4.3.5

Deprecation warning shows up regardless of which overloaded signature you use. Should not show up for the last two signatures.

import { interval } from 'rxjs';
import { tap } from 'rxjs/operators';

interval(5000)
  .pipe(
     tap(console.log), // "deprecated - Use an observer instead of a complete callback"
     tap({ next: console.log, error: console.log, complete: console.log }) // "deprecated
  )

rxjs/tap.d.ts:

import { MonoTypeOperatorFunction, PartialObserver } from '../types';
/** @deprecated Use an observer instead of a complete callback */
export declare function tap<T>(next: null | undefined, error: null | undefined, complete: () => void): MonoTypeOperatorFunction<T>;
/** @deprecated Use an observer instead of an error callback */
export declare function tap<T>(next: null | undefined, error: (error: any) => void, complete?: () => void): MonoTypeOperatorFunction<T>;
/** @deprecated Use an observer instead of a complete callback */
export declare function tap<T>(next: (value: T) => void, error: null | undefined, complete: () => void): MonoTypeOperatorFunction<T>;
export declare function tap<T>(next?: (x: T) => void, error?: (e: any) => void, complete?: () => void): MonoTypeOperatorFunction<T>;
export declare function tap<T>(observer: PartialObserver<T>): MonoTypeOperatorFunction<T>;
@SpeedoPasanen SpeedoPasanen changed the title This is still happening at least with the tap function from rxjs. VSCode 1.58.2. TS version in project: 4.3.5 Unexpected deprecation warning with the tap function from rxjs Aug 5, 2021
@SpeedoPasanen SpeedoPasanen changed the title Unexpected deprecation warning with the tap function from rxjs Unexpected deprecation warning with tap from rxjs Aug 5, 2021
@RyanCavanaugh
Copy link
Member

I don't see this. My tap.d.ts doesn't match yours; seems like you have a different version of rxjs (I just installed latest). Please provide an isolated repro that doesn't need external dependencies.
image

import { interval } from 'rxjs';
import { tap } from 'rxjs/operators';

interval(5000)
    .pipe(
        tap(console.log), // not deprecated
        tap({ next: console.log, error: console.log, complete: console.log }) // not deprecated
    )

interval(5000)
    .pipe(
        tap(null, null, null), // deprecated
    );

@RyanCavanaugh RyanCavanaugh added the External Relates to another program, environment, or user action which we cannot control. label Aug 5, 2021
@SpeedoPasanen
Copy link
Author

SpeedoPasanen commented Aug 6, 2021

To be more clear, i'm talking about just the comment showing up at wrong times. Actually VSCode only strikes over the correct, deprecated one. But still the comment shows up when using any of these signatures.

I noticed that the position of the deprecated signature matters. If it's the first signature in the file, the comment will show up when hovering any of these function calls.

index.ts

/** @deprecated This signature is deprecated */
export declare function fn();
export declare function fn(a: any);
export declare function fn(a: any, b: any);

fn();
fn(1);
fn(1, 2);

image

If I simply swap places of the first and second signature, the comment is correctly not visible when hovering over fn(1) or fn(1, 2).

export declare function fn(a: any);
/** @deprecated This signature is deprecated */
export declare function fn();
export declare function fn(a: any, b: any);

fn();
fn(1);
fn(1, 2);

image

package.json

{
  "name": "repro",
  "version": "1.0.0",
  "description": "",
  "main": "index.ts",
  "scripts": {
    "start": "ts-node index.ts"
  },
  "author": "",
  "license": "ISC",
  "devDependencies": {
    "ts-node": "^10.1.0",
    "typescript": "^4.3.5"
  }
}

@SpeedoPasanen
Copy link
Author

SpeedoPasanen commented Aug 6, 2021

It is confusing to developers. I've seen people (including me) scratch their head for hours trying to figure out why they see this comment when they're using a non-deprecated thing correctly.

@SpeedoPasanen
Copy link
Author

SpeedoPasanen commented Aug 6, 2021

And actually this is not about @deprecated at all. It's just that any comment, if it's before all signatures, gets shown for all the signatures below, unless there's another comment below which overrides the one on top. And again, simply just making it so that the first signature doesn't have a comment acts as a work-around.

/** This comment will show up for the first two signatures */
export declare function fn(); 
export declare function fn(a: any);
/** This will override the first comment for the last signature */
export declare function fn(a: any, b: any);

fn(); // Hover: "This comment will show up for the first two signatures"
fn(1); // Hover: "This comment will show up for the first two signatures"
fn(1, 2); // Hover: "This will override the first comment for the last signature"

And now that I think about it more, is this actually intended behavior? Is the first signature always the "default", from which comments are supposed to be copied to all the other signatures, unless overridden?

@typescript-bot
Copy link
Collaborator

This issue has been marked as 'External' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@RyanCavanaugh RyanCavanaugh reopened this Aug 12, 2021
@RyanCavanaugh RyanCavanaugh added Needs Investigation This issue needs a team member to investigate its status. and removed External Relates to another program, environment, or user action which we cannot control. labels Aug 12, 2021
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 4.5.0 milestone Aug 12, 2021
@sandersn sandersn changed the title Unexpected deprecation warning with tap from rxjs Quick info defaults to showing jsdoc from first overload Aug 13, 2021
@sandersn sandersn added Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript and removed Needs Investigation This issue needs a team member to investigate its status. labels Aug 13, 2021
@sandersn sandersn changed the title Quick info defaults to showing jsdoc from first overload Quick info shouldn't default to showing jsdoc from first overload Aug 13, 2021
@sandersn
Copy link
Member

This behaviour is intended because I believe people more often want to document all signatures at once when they write a jsdoc before the initial signature. But it's worth revisiting this. What I think we'd need to move this suggestion forward:

  1. An argument that @deprecated is different from other tags, and/or shouldn't be copied to non-initial signatures. Maybe no tags should be carried over.

OR

  1. A sample of usage that shows that people always intend to associate an initial jsdoc with a particular signature, not all of them at once. In other words, that the current design is wrong.

@sandersn sandersn removed this from the TypeScript 4.5.0 milestone Aug 13, 2021
@SpeedoPasanen
Copy link
Author

SpeedoPasanen commented Aug 20, 2021

Now that I thinked this through, my opinion is that all comments should carry over, so they work consistently, deprecation or not. Library authors just should be aware and keep this in mind. RXJS has already fixed this for subscribe, but not tap.

Thank you @sandersn and my love and appreciation to everyone working on TypeScript <3

Edit: Seems this is fixed in RXJS 7. It's just that the latest Angular still ships with 6. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

4 participants