deno_doc icon indicating copy to clipboard operation
deno_doc copied to clipboard

Documentation should take class extensions into consideration

Open andreespirela opened this issue 3 years ago • 7 comments

As of right now, deno doc does not respect extensions. This means

export class B { 
   public id: number;
}
export class A extends B { 
  public userName: string;
}

if I run deno doc for class A, it will only document the property userName but will not for id part of the class extension.

deno doc should be able to read this, and respect the extensions of extensions (example, B extends C)

andreespirela avatar Aug 24 '21 19:08 andreespirela

This is also true (and possibly worse) for implements. Given

interface TreeNode {
  /** ID of parent */
  parent: string
}

/** Some Doc explaining BusinessUnit */
class BusinessUnit implements TreeNode {
  public parent: string

  constructor(parent: string) {
    this.parent = parent
  }
}

deno docing BusinessUnit will show parent: string but without any documentation, which is kind of the worst of both worlds.

I would be happy to try to build a solution, but we would have to decide for a solution.

I think no matter how we display it, ClassDef and InterfaceDef would probably need some form of inheritedMembers field to hold inherited methods and properties from implements and extends. For the JSDoc of those members, my intuition would be to start looking for a JSDoc block at the class/interface itself and walk up the inheritance tree until a JSDoc block is found and use that (or none if none was found). That allows subclasses to override JSDoc on inherited members if needed, but inherit JSDoc automatically if not (which will probably be the case most of the time).

I think displaying inherited members under some separate heading (on doc.deno.land and in deno doc output), together with their direct source (something like "inherited from X") and JSDoc might make sense.

Any thoughts?

LionC avatar Sep 28 '21 17:09 LionC

There was a feedback from std users that inherited properties not being shown in API docs is confusing https://github.com/denoland/deno_std/issues/4353

Can we consider listing all properties including inherited ones?

kt3k avatar Mar 04 '24 06:03 kt3k

Can we consider listing all properties including inherited ones?

I think we should not list all properties, but instead hyperlink to the associated symbol (e.g. type alias/interface/class/etc.). This will prevent a significant amount of noise.

Ref: https://github.com/denoland/deno_std/issues/4353#issuecomment-1962927306

jsejcksn avatar Mar 04 '24 14:03 jsejcksn

Hyperlink is already available in doc website (jsr). For example, see https://jsr.io/@std/csv/doc/~/CsvParseStreamOptions

This will prevent a significant amount of noise.

I'm not sure I agree these are noises.

If someone comes to the page for CsvParseStreamOptions, then they probably want to know the all available options in CsvParseStreamOptions. Currently they need to see 2 separate pages to know all options, and that is not convenient for them.

kt3k avatar Mar 04 '24 15:03 kt3k

I'm not sure I agree these are noises.

If someone comes to the page for CsvParseStreamOptions, then they probably want to know the all available options in CsvParseStreamOptions.

I agree that in cases where the interface is fully bespoke, a reader would certainly want to understand the shape of the type in full (because it is completely unfamiliar).

However, there are many cases where types are derived from other types (e.g. extension, inheritance, intersection, etc.) — and in many cases the existing types are already familiar, so the expanded type information serves no purpose (it's only noise). Here's a contrived example for illustration:

TS Playground

type LinkedNode<T> = T & {
  child?: LinkedNode<T>;
  parent?: LinkedNode<T>;
};

type ResponseNode = LinkedNode<Response>;
/*   ^?
{
  arrayBuffer: () => Promise<ArrayBuffer>;
  blob: () => Promise<Blob>;
  readonly body: ReadableStream<Uint8Array> | null;
  readonly bodyUsed: boolean;
  child?: LinkedNode<Response>;
  clone: () => Response;
  formData: () => Promise<FormData>;
  readonly headers: Headers;
  json: () => Promise<any>;
  readonly ok: boolean;
  parent?: LinkedNode<Response>;
  readonly redirected: boolean;
  readonly status: number;
  readonly statusText: string;
  text: () => Promise<string>;
  readonly type: ResponseType;
  readonly url: string;
}
*/

In the example above, Response is a standard, built-in type, and well-known by many developers. I think it is noise to include all of the fields in Response, and — similarly — I think it is noise to include the properties from LinkedNode because they both already exist as named types and can be referenced. If the reader is already familiar with either/both of them, then inlining all of the content makes it more difficult to understand which fields are unique to the type in question (there are none in the example, but other types might have them).

Currently they need to see 2 separate pages to know all options, and that is not convenient for them.

If the user is not familiar with Response, then perhaps they're also unfamiliar with FormData, which is in the return type of the field formData:

formData: () => Promise<FormData>;

It's also inconvenient for the user to click FormData to see its type information, right? It seems arbitrary that we would expand and inline all of the type information for the top-level fields, but not any of the type information for nested fields. I'm not sure that the convenience reasoning is scalable.

jsejcksn avatar Mar 04 '24 16:03 jsejcksn

Ok. The above case looks like a valid case where showing inherited props can be noisy.

Then how about showing some text link saying show inherited properties at the bottom of properties? and when a user clicks that button, the inherited props are expanded there. That would cater for both needs, I think

kt3k avatar Mar 05 '24 04:03 kt3k

Then how about showing some text link saying show inherited properties at the bottom of properties? and when a user clicks that button, the inherited props are expanded there. That would cater for both needs, I think

I think that's a great feature idea!

jsejcksn avatar Mar 05 '24 04:03 jsejcksn