Started adding JSDoc #984

shashkovdanil · 2023-09-07T23:08:27Z

Even though I love TypeScript, I respect the decision made by the owners. I can admit that sometimes TypeScript causes a headache and you have to use as/any/unknown. It's also hard to read and understand what is happening in code like this.

export type GetKeysByValue<T extends Values> = {
  [K in keyof ItemMap]: ItemMap[K] extends { value: infer U } ? (U extends T ? K : never) : never;
}[keyof ItemMap];

However, it must be acknowledged that the code's readability has decreased. It will be harder for users to understand how to use turbo, and for other contributors to understand what functions and methods are doing. And instead of arguing, I prefer to find compromises. Therefore, I decided to combine the best of both worlds and started adding JSDoc. You can see what this looks like in this PR. I don't want to waste my time in vain, so I'm putting this up for public judgment. If the maintainers approve this PR, I will cover the remaining code with JSDoc.

seanpdoyle · 2023-09-08T01:13:36Z

src/core/session.js

+  /**
+   * Connects a new stream source.
+   *
+   * @param {any} source - The new stream source to connect.


This is either EventSource | WebSocket

seanpdoyle · 2023-09-08T01:13:59Z

src/core/session.js

+  /**
+   * Disconnects an existing stream source.
+   *
+   * @param {any} source - The stream source to disconnect.


This is either EventSource | WebSocket

seanpdoyle · 2023-09-08T01:17:27Z

Thank you for opening this. I think this is an improvement over what's currently available.

I'm in favor of documentation of any kind, especially documentation that brings typed specificity.

I worry that the core team's disinterest in TypeScript might extend to a disinterest in typed code comments. Even the most correctly typed JSDoc comments risk becoming useless without the support from the tooling that enforces them and the human commitment to adhere to the results of the tooling.

Could this pull request also introduce tooling for CI and local development?

Does the core team have interest in incorporating that tooling into the project?

WagnerMoreira · 2023-09-08T01:27:11Z

I'm highly interested in making contributions here and adding JSDoc over the whole codebase if this move is accepted.

@seanpdoyle

Could this pull request also introduce tooling for CI and local development?

you mean things like using eslint-plugin-jsdoc and checking for the presence and correctness of JSDoc comments?

FroggyPanda · 2023-09-08T01:34:48Z

I would also like to help out with reading types for this project. However, I would like one of the main contributors, like @.dhh, to say if they're willing to add this PR so our time doesn't get wasted reading something that was removed.

DASPRiD · 2023-09-08T01:46:37Z

src/core/session.js

+   * Initiates a visit to a specified location, with optional parameters.
+   *
+   * @param {string} location - The location to visit.
+   * @param {Object} options - Optional parameters for the visit.


Would it make sense to define the options properties? This can be done via @typedef.

DASPRiD · 2023-09-08T01:48:18Z

src/core/session.js

+  /**
+   * Sets the form mode.
+   *
+   * @param {string} mode - The form mode to set, either "on" or "off".


This could be typed as @param {('on'|'off')} mode.

DASPRiD · 2023-09-08T01:50:33Z

src/core/session.js

+   *
+   * @private
+   * @param {HTMLFormElement} form - The form element being submitted.
+   * @param {HTMLElement} submitter - The element that triggered the form submission.


submitter is optional, it should therefore be typed as @param {HTMLElement} [submitter].

DASPRiD · 2023-09-08T01:58:46Z

This is a good idea. I did check out your branch and ran tsc to generate type definitions from this, which produces quite usable results for the files you touched:

npx -p typescript tsc src/**/*.js --declaration --allowJs --emitDeclarationOnly --outDir types

shashkovdanil · 2023-09-08T08:35:06Z

Hey guys! So I'm waiting for answer from maintainers, if they're interested in it, I'll add infrastructure like eslint and continue writing documentation

WebReflection · 2023-09-08T10:37:16Z

imho, "this is the way" (cit)

the benefits of JSDoc TS are many:

the tsc build step is not strictly necessary anymore, but it can always be used to --check from time to time via npx without having it as dependency
JSDoc itself encourage better documentation, not just types, and there are tools able to use that documentation to generate out of the box proper static pages anyone can read or crawl whenever it's needed
if focused only on the public API, the rest of the code will be still enjoyable (for the team) and maintenance would be kept minimal as public API signatures are all dependent projects need to have TS work "best", and TS works already very well with JSDoc

last, but not least, I love the fact somebody proposed something instead of insulting people involved in this project (not everyone, but what I've read these days show how TS community can be way more toxic and hostile than anything happened in here).

I hope maintainers will give this PR a chance.

afcapel · 2023-09-08T11:07:03Z

@shashkovdanil thanks for the civil and constructive approach to the PR, I really appreciate it.

What we found that works for us to document Turbo is having a documentation project that you can access online.

Most of the public Turbo APIs are HTML annotations (data- attributes, mostly), custom elements and events. Having a documentation website allows us to document all of those together in a consistent way.

There are a few cases where you might want to call a Turbo public API directly from your code, like Turbo.visit or Turbo.cache.clear. For those cases we were thinking to ship some type annotations to DefinitelyTyped, so people can enjoy their types too.

WagnerMoreira · 2023-09-08T01:29:48Z

src/core/session.js

@@ -40,6 +44,9 @@ export class Session {
  started = false
  formMode = "on"

+  /**
+   * Starts all observers and services if not already started.


what about adding a return here?

* @returns {void}

WagnerMoreira · 2023-09-08T02:02:15Z

src/core/session.js

+   * Initiates a visit to a specified location, with optional parameters.
+   *
+   * @param {string} location - The location to visit.
+   * @param {Object} options - Optional parameters for the visit.


since options is optional, you can use square brackets around the description

* @param {Object} [options] - Optional parameters for the visit.

shashkovdanil · 2023-09-08T12:04:19Z

@afcapel Hi, JSDoc is not only good for users who will use the public API, but it is also good for contributors. If the documentation covers all this and JSDoc is not useful, I think you can close the PR

Gravy59 · 2023-09-08T23:05:09Z

Hi, you might consider using ts-to-jsdoc or similar to expedite the process.

marcoroth · 2023-09-09T16:41:11Z

Here's an attempt to migrate the whole codebase to JSDoc using the above mentioned ts-to-jsdoc package: marcoroth@1ea3bfc

The generated code is not 100% perfect in all cases, but I would be more than happy to polish this by hand if we decide to continue moving forward with this.

I think having the comments/documentation is valuable for contributors to see and understand how the internals work. This is also why it's not really a solution to just push up the types to DefinitelyTyped. It's not too valuable for users using Turbo and consuming the exported types the packages ships with, it's about the context and documentation we lost about the internals.

TechStudent10

This is a great step forward. If the devs (looking at you DHH) really want to ditch TypeScript, I think JSDoc is a great in-between

shashkovdanil · 2023-09-09T21:01:50Z

@afcapel hey, should I continue this?

ChristianECG · 2023-09-10T17:44:18Z

It appears that the team's [official stance][] is that documentation for the package's Custom Events should reside on a consumer-facing website. The documentation for Custom Events already exists, but doesn't provide particulars for the shapes of the most of the `event.details` objects. This commit brings more structure to the page, namely: * groups events by their source * promotes event names to deeply-linkable headings * adds HTML tables to describe each event's `event.detail` properties, along with type information [official stance]: hotwired/turbo#984 (comment)

afcapel · 2023-09-12T09:39:21Z

@shashkovdanil I'm afraid we're not interested in bringing back the type system, even if it's in the form of comments.

But thanks again for the constructive approach to the discussion.

It appears that the team's [official stance][] is that documentation for the package's Custom Events should reside on a consumer-facing website. The documentation for Custom Events already exists, but doesn't provide particulars for the shapes of the most of the `event.details` objects. This commit brings more structure to the page, namely: * groups events by their source * promotes event names to deeply-linkable headings * adds HTML tables to describe each event's `event.detail` properties, along with type information [official stance]: hotwired/turbo#984 (comment)

started to add JSDoc

45b8fe0

seanpdoyle reviewed Sep 8, 2023

View reviewed changes

DASPRiD reviewed Sep 8, 2023

View reviewed changes

w3bdesign approved these changes Sep 8, 2023

View reviewed changes

WagnerMoreira reviewed Sep 8, 2023

View reviewed changes

ArnavK-09 approved these changes Sep 8, 2023

View reviewed changes

FroggyPanda approved these changes Sep 8, 2023

View reviewed changes

TechStudent10 approved these changes Sep 9, 2023

View reviewed changes

This was referenced Sep 10, 2023

Document Type information for Custom Events hotwired/turbo-site#145

Merged

Generate hotwired/turbo-site content from hotwired/turbo source #997

Open

afcapel closed this Sep 12, 2023

seanpdoyle mentioned this pull request Sep 12, 2023

[Deprecation]: Remove internal objects from public API #1000

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Started adding JSDoc #984

Started adding JSDoc #984

shashkovdanil commented Sep 7, 2023

seanpdoyle Sep 8, 2023

seanpdoyle Sep 8, 2023

seanpdoyle commented Sep 8, 2023 •

edited

Loading

WagnerMoreira commented Sep 8, 2023

FroggyPanda commented Sep 8, 2023

DASPRiD Sep 8, 2023

DASPRiD Sep 8, 2023

DASPRiD Sep 8, 2023

DASPRiD commented Sep 8, 2023

shashkovdanil commented Sep 8, 2023

WebReflection commented Sep 8, 2023 •

edited

Loading

afcapel commented Sep 8, 2023 •

edited

Loading

WagnerMoreira Sep 8, 2023

WagnerMoreira Sep 8, 2023

shashkovdanil commented Sep 8, 2023

Gravy59 commented Sep 8, 2023

marcoroth commented Sep 9, 2023

TechStudent10 left a comment

shashkovdanil commented Sep 9, 2023

ChristianECG commented Sep 10, 2023

afcapel commented Sep 12, 2023

Started adding JSDoc #984

Started adding JSDoc #984

Conversation

shashkovdanil commented Sep 7, 2023

seanpdoyle Sep 8, 2023

Choose a reason for hiding this comment

seanpdoyle Sep 8, 2023

Choose a reason for hiding this comment

seanpdoyle commented Sep 8, 2023 • edited Loading

WagnerMoreira commented Sep 8, 2023

FroggyPanda commented Sep 8, 2023

DASPRiD Sep 8, 2023

Choose a reason for hiding this comment

DASPRiD Sep 8, 2023

Choose a reason for hiding this comment

DASPRiD Sep 8, 2023

Choose a reason for hiding this comment

DASPRiD commented Sep 8, 2023

shashkovdanil commented Sep 8, 2023

WebReflection commented Sep 8, 2023 • edited Loading

afcapel commented Sep 8, 2023 • edited Loading

WagnerMoreira Sep 8, 2023

Choose a reason for hiding this comment

WagnerMoreira Sep 8, 2023

Choose a reason for hiding this comment

shashkovdanil commented Sep 8, 2023

Gravy59 commented Sep 8, 2023

marcoroth commented Sep 9, 2023

TechStudent10 left a comment

Choose a reason for hiding this comment

shashkovdanil commented Sep 9, 2023

ChristianECG commented Sep 10, 2023

afcapel commented Sep 12, 2023

seanpdoyle commented Sep 8, 2023 •

edited

Loading

WebReflection commented Sep 8, 2023 •

edited

Loading

afcapel commented Sep 8, 2023 •

edited

Loading