rxjs icon indicating copy to clipboard operation
rxjs copied to clipboard

Incorrect Intellisense description for `forkJoin`

Open davimiku opened this issue 3 years ago • 6 comments

Describe the bug

In the example code below, the hover definition for forkJoin displays a description about the any type, even when the any type is not used.

You have passed any here, we can't figure out if it is an array or an object, so you're getting unknown. Use better types.

In the example below, TypeScript inferred a tuple type [number, number].

image

Expected behavior

Hover definition for forkJoin displays the main/standard documentation definition for forkJoin.

Reproduction code

import { forkJoin, of } from 'rxjs'

forkJoin([of(1), of(2)])

Reproduction URL

No response

Version

7.5.7

Environment

Visual Studio Code (latest version - v1.7.2) TypeScript bundled with VS Code (v4.8.4)

Additional context

No response

davimiku avatar Oct 25 '22 20:10 davimiku

I did some research and believe that this is due to the overload for forkJoin(any) being the first and only overload with a JSDoc comment. Since the overload that is actually being used has no JSDoc comment, IntelliSense is defaulting to the JSDoc of this first overload (even if it doesn't have any).

After some research, I came up with two possible solutions:

  1. Write JSDoc comments for all overloads. This might be very repetitive and would be very annoying to maintain.
  2. Slightly re-arrange the order of the overloads and then (optionally) add only one generic JSDoc comment. This would solve the JSDoc problem as IntelliSense seems to fall back on the first overload's JSDoc, so making the first overload be the non-any one would allow us to only add a comment here that is then displayed for all non-any overloads. And it is possible (contrary to the comment in forkJoin.ts) because there are non-generic overloads (e.g. forkJoin(sources: readonly [])) that TypeScript chooses not to match with any for a reason which I haven't researched yet (I can if it's desired). So choosing one of these non-generic overloads to be the first would preserve the type definitions while having IntelliSense fall back on a non-any overload for JSDoc.

I created a sandbox that shows all of this in more detail. Please reach out if any of this is unclear.

If possible, I would like to implement a PR for this issue. In order to do that, I will just need to know which approach you'd like me to take and if you want the generic comment from the actual implementation to be used or if no JSDoc should be provided :) (I would suggest using the comment from further below)

Also, I can imagine the same issue exists with other functions. Should I look around and fix it everywhere I can find it in the same PR or should that be left for another PR?

LBBO avatar Dec 28 '22 14:12 LBBO

Would someone care to comment on this? Perhaps @benlesh? (Sorry if you're not the right person to ping here! In that case, I'd appreciate it if you could let me know if and who I can ping instead)

LBBO avatar Jan 17 '23 16:01 LBBO

The same thing happens with combineLatest.

I would love to see some action on this.

dangrussell avatar Feb 01 '23 19:02 dangrussell

Like I said, I'd be happy to open a PR but I would like to avoid wasting time on an approach the maintainers don't want or something similar.

LBBO avatar Feb 01 '23 21:02 LBBO