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

JSDoc Typedef Syntax for Overloadable Constructors #55916

Closed
5 tasks done
ITenthusiasm opened this issue Sep 29, 2023 · 5 comments
Closed
5 tasks done

JSDoc Typedef Syntax for Overloadable Constructors #55916

ITenthusiasm opened this issue Sep 29, 2023 · 5 comments
Labels
Question An issue which isn't directly actionable in code

Comments

@ITenthusiasm
Copy link

ITenthusiasm commented Sep 29, 2023

🔍 Search Terms

constructor jsdoc overload

✅ Viability Checklist

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of our Design Goals: https://github.com/Microsoft/TypeScript/wiki/TypeScript-Design-Goals

⭐ Suggestion

Currently, with pure TypeScript, it's possible to create an overloaded, generic constructor type. (The benefits of overloaded generic constructors are better detailed in #40451.) However, there is no similar concept available in JSDocs. After migrating a TS project to JSDocs, I found this to be a hindrance. (There are some workarounds, but they still negatively impact user experience surrounding imports.)

It would be great if this use case was supported in JSDocs land. Alternatively, if #40451 was resolved, I'm assuming that this issue would be inherently be resolved as well. Rich Harris made a compelling case for using JSDocs in his interview with the Primeagen and his interview with Kev from Svelte Society. It seems there are more library authors interested in reaching for JSDocs (through TS) for simplicity (myself included); so greater support in situations like these would be fantastic.

📃 Motivating Example

I'm appealing to #40451 and the example in #52585 here. If I want to create a DOM-watcher class in TypeScript that will have a consistent, unchanging interface and that can be constructed in different ways, I need a generic constructor:

type EventType = keyof DocumentEventMap;
type ObserverEvent<T extends EventType> = DocumentEventMap[T];
type ObserverEventListener<T extends EventType> = (event: ObserverEvent<T>) => unknown;

/* -------------------- Base Observer -------------------- */
interface BaseObserverConstructor {
  new <T extends EventType>(type: T, listener: ObserverEventListener<T>): BaseObserver;
  new <T extends ReadonlyArray<EventType>>(types: T, listener: ObserverEventListener<T[number]>): BaseObserver;
}

interface BaseObserver {
  observe(element: HTMLElement): void;
  unobserve(element: HTMLElement): void;
  disconnect(): void;
}

export const BaseObserver: BaseObserverConstructor = class<T extends EventType | ReadonlyArray<EventType>> {
  constructor(typeOrTypes: T, listener: ObserverEventListener<EventType>) {
    // Do something with arguments
  }

  observe(element: HTMLElement): void {
    // Observe the element
  }

  unobserve(element: HTMLElement): void {
    // Undo the observation
  }

  disconnect(): void {
    // `unobserve` everything
  }
};

This works fine in TS, but it's incompatible with JSDocs -- causing friction during migration. If JSDoc support was added, perhaps we'd probably have something like the following:

/** @typedef {keyof DocumentEventMap} EventType */
/** @template {EventType} T @typedef {DocumentEventMap[T]} ObserverEvent */
/** @template {EventType} T @typedef {(event: ObserverEvent<T>) => unknown} ObserverEventListener */

/* -------------------- Base Observer -------------------- */
/**
 * @constructor BaseObserverConstructor
 * // Define overloads that create a `BaseObserver` instance
 */

/**
 * @typedef {Object} BaseObserver
 * // Define types
 */

/** 
 * @template {EventType | ReadonlyArray<EventType>} T
 * @type {BaseObserverConstructor}
 */
export const BaseObserver = class {
  /**
   * @param {T} typeOrTypes
   * @param {ObserverEventListener<EventType>} listener
   */
  constructor(typeOrTypes, listener) {
    // Do something with arguments
  }

  /** @param {HTMLElement} element @returns {void} */
  observe(element) {
    // Observe the element
  }

  /** @param {HTMLElement} element @returns {void} */
  unobserve(element) {
    // Undo the observation
  }

  /** @returns {void} */
  disconnect() {
    // `unobserve` everything
  }
};

That said, this can also be solved by resolving #40451. Resolving said issue would remove the need for any additional TS-specific JSDoc syntax. However, if that is not up for consideration, then a JSDoc syntax that mimics TypeScript's behavior would be very helpful.

💻 Use Cases

1) What do you want to use this for?

The example above is probably the clearest example of what I need this for. This is for a project that I'm actively working on.

2) What shortcomings exist with current approaches?

The current shortcoming is that this isn't possible for those only wanting to use JSDocs. This creates some minor friction in how a project structures its exports (in package.json) and how a project structures its filesystem. (I didn't require extra .d.ts files before migrating to JSDocs.) It also separates a class method's documentation (defined in an interface in a .d.ts file) from its implementation (defined in a js file) -- resulting in lesser maintainability.

3) What workarounds are you using in the meantime?

The current workaround is to create a separate <FILE_NAME>Types.d.ts file that has the TS Generic Constructor defined. The constructor can be imported from the types file by the JS file. This would need to be done for every JavaScript file. Alternatively, a global types file for the application/library being built can be created, and the types would be imported collectively from there instead.

This does add some complexity for library maintainers using package.json's exports property, however.

@DanielRosenwasser
Copy link
Member

If you want a generic constructor, you can write

/**
 * @template T
 */
class C {
    /**
     * @template T
     * @overload
     * @param {T} value
     * @return {C<T>}
     */

    /**
     * @template T
     * @overload
     * @param {T} first
     * @param {T} second
     * @return {C<T>}
     */

    /**
     * @param {T} first
     * @param {T} [second]
     */
    constructor(first, second) {
    }
}

const x = new C(123);
//    ^?

const y = new C("hello", "world");
//    ^?

const y = new C(123, "hello");
//    ^?

See the playground.

It is questionable whether that's the right way to declare constructor overloads though. Ideally you could write something like this, but it results in errors.

/**
 * @template T
 */
class C {
    /**
     * @overload
     * @param {T} value
     */

    /**
     * @overload
     * @param {T} first
     * @param {T} second
     */

    /**
     * @param {T} first
     * @param {T} [second]
     */
    constructor(first, second) {
    }
}

const x = new C(123);
//              ~~~
// Argument of type 'number' is not assignable to parameter of type 'T'.
//  'T' could be instantiated with an arbitrary type which could be unrelated to 'number'.

const y = new C("hello", "world");
//              ~~~~~~~
// Argument of type 'string' is not assignable to parameter of type 'T'.
//  'T' could be instantiated with an arbitrary type which could be unrelated to 'string'.

(playground link)

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Sep 29, 2023

Looking more closely at your code, I realize you're asking about something else.

You can just express BaseObserverConstructor in the following way.

/** @typedef {keyof DocumentEventMap} EventType */
/** @template {EventType} T @typedef {DocumentEventMap[T]} ObserverEvent */
/** @template {EventType} T @typedef {(event: ObserverEvent<T>) => unknown} ObserverEventListener */

/**
 * @typedef {Object} BaseObserver
 */

/**
 * @typedef {{
 *   new <T extends EventType>(type: T, listener: ObserverEventListener<T>): BaseObserver;
 *   new <T extends ReadonlyArray<EventType>>(types: T, listener: ObserverEventListener<T[number]>): BaseObserver;
 * }} BaseObserverConstructor
 */

Playground Link

@ITenthusiasm
Copy link
Author

@DanielRosenwasser The example that you gave me seems to fail when the various templates are identified with the same letter. Is that expected? Playground Link

Additionally, the comments for the @overload JSDocs don't seem appear when the constructor overloads are called. (I don't see the descriptions on hover.) Is that normal as well?

I actually prefer the first example you showed me, where I can simply use class directly. I suppose in the meantime, I can use the typedef example that you provided. But I only suggested the typedef approach because I didn't know syntax was possible/allowed when using class directly. In the past, I've received TS errors stating that constructors cannot be generic.

@ITenthusiasm
Copy link
Author

It seems like this is a bug with overloads, potentially. Will open a separate issue in that case.

@DanielRosenwasser
Copy link
Member

Yeah, in TS itself, construct signatures can't add more generic type parameters than their containing class.

In the JSDoc version, it's an irregularity that overload signatures permit them.

The doc comments being lost is a valid bug as well if you'd like to file something there.

ITenthusiasm added a commit to enthusiastic-js/form-observer that referenced this issue Sep 30, 2023
…rver`

This allows us to have our documentation and our types
in the same place! Hopefully it'll make developer experience
better in the long run. We'll make this update for the
`FormValidityObserver` as well.

HUGE thanks to @DanielRosenwasser! (See his current
solution for JSDocs in microsoft/TypeScript#55916.)

This is GAME CHANGING.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question An issue which isn't directly actionable in code
Projects
None yet
Development

No branches or pull requests

2 participants