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

Add JSDoc annotations and documentation to the JS code #2892

Closed
wants to merge 5 commits into from

Conversation

tgroshon
Copy link

@tgroshon tgroshon commented Oct 27, 2023

Included are JSDoc annotations for every class, method, and function of all the JavaScript files in /assets/js/phoenix_live_view/.

Key Points and Callouts

  1. No code behavior was changed: changes restricted to only whitespace and comments (either JSDoc or ESLint).
    1. Tests were run to verify
    2. ESLint static checks verified (NOTE: many files had existing lint errors; when they were whitespace related, I fixed
      them. I added some eslint-disable comments in other spots. Otherwise I ignored them. Some existing errors remain because they required changing code but I didn't want to mix code changes [even minor] with an already large change).
  2. This change does not actually install or run JSDoc, TypeScript compiler, or any other tooling, or otherwise change the project configuration.
  3. JSDoc comments follow the TypeScript flavor and generally stick to only that subset of JSDoc that is understood by TypeScript (see write-up on TypeScript website here: "JSDoc Reference").
  4. I tried to maintain the expectations and guidelines outlined in the main Phoenix project's CONTRIBUTING.md file about contributing documentation.
  5. The new annotations greatly enhance intellisense, autocompletion, and other LSP/IDE features for anyone using a TypeScript-lsp compatible editor.
  6. Existing comments were maintained either in-place or moved to the head of a JSDoc annotation if it felt appropriate. Most in-line comments within a function were left as-is.
  7. The most difficult files to annotate (unsurprising to anyone familiar with the code) and thus needing the most review on their explanations and type signatures are as follows:
    1. view.js
    2. live_socket.js
    3. dom_patch.js
    4. rendered.js
  8. If the functionality of some JS code seemed clear, I added basic explanation.
  9. Most of the docstring explanations are ok, but some I'm not super happy with and could definitely be better. I was able to improve on some after I read and understood other parts of the code and came back to rewrite them. Some I just added type annotations to, but no explanation because either I (a) wasn't sure or (b) the implementation was self-evident.
  10. The PR is obviously huge. I kept it all together because there's a good deal of imported type definitions between files, and the relations are obvious when they're submitted all together. It would be possible to separate this out into a handful of PRs if the review is too onerous.
  11. PLEASE make any suggestions for better (a) explanations, (b) type signatures, (c) or whatever.

Future

  1. Naturally, could actually run JSDoc to extract annotations; maybe even create a process that slurps up the JSDoc into the HexDocs 🤯
    1. Most of this code is obviously internal implementation details for LiveView, so publishing all this externally may not be hugely beneficial.
  2. Trivial to configure a NPM script or build step that uses the TypeScript compiler to generate Type Declaration files (see here) and expose them alongside the build artifacts.
    1. Same as previous point; may not be much benefit in publishing type declarations for primarily private code.
    2. HOWEVER: this could be valuable for providing enhanced intellisense and optional type-checking for users writing their own View Hook callbacks. That's actually how I got the itch for this. I was writing my own Hook in TypeScript and spent some time deriving the interface from the Hex docs for LiveView for my own internal use. Most of that original documentation and type annotation work now lives in view_hook.js.
  3. I'd really like to correctly annotate and declare the type definition in rendered.js for the over-the-wire live view protocol for communicating rendered diff trees. I spent a few hours trying to reverse engineer it from (a) code, (b) tests, and (c) this fantastic article by Jose', but... I couldn't quite nail it down, so I used a very loose type. This would probably be easy for someone more familiar with problem domain and implementation.

Screenshots!

Highlighting existing code for explanations

Screenshot from 2023-10-27 14-18-56

Intellisense Suggestions

Screenshot from 2023-10-27 14-39-46

Example of how a file looks

Screenshot from 2023-10-27 14-54-40

Copy link
Contributor

@03juan 03juan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some grammatical fixes and attempts at completing descriptions. Great effort!

assets/js/phoenix_live_view/js.js Outdated Show resolved Hide resolved
assets/js/phoenix_live_view/live_uploader.js Outdated Show resolved Hide resolved
assets/js/phoenix_live_view/rendered.js Outdated Show resolved Hide resolved
assets/js/phoenix_live_view/dom.js Outdated Show resolved Hide resolved
assets/js/phoenix_live_view/live_socket.js Outdated Show resolved Hide resolved
@tgroshon
Copy link
Author

Thanks! I'll make corrections today.

@03juan
Copy link
Contributor

03juan commented Oct 29, 2023

Just a quick note:

The @param {import('./view.js').default} view or @property {(this: ViewHook) => void} [mounted] are TypeScript-specific syntax and might not be compatible with standard JSDoc or non-TS IDEs.

Future Options for Library Authors:

  • Keep as-is if the primary goal is to support TypeScript-enabled IDEs like VSCode.
  • Switch to standard JSDoc annotations, which might require creating a runtime dependency to keep JSDoc happy.
  • Consider adding TypeScript as a dev dependency to generate type definitions, which would make this compatible across the board but adds a new dependency.

Just something to think about for future-proofing the docs!

(out of interest I ran plain jsdoc against the directory and I believe all the errors are related to the TS issue)

❯ jsdoc -r . 2>&1 | grep 'ERROR' | wc -l
278

@tgroshon
Copy link
Author

@03juan Thanks for all the comments! I've made made the updates you suggested, and also found a few other spots that needed fixed.

Also, Yes, I believe you're right about JSDoc and TS syntax. It's something I tried to proactively call out in my "Key Points and Callouts" item 3:

JSDoc comments follow the TypeScript flavor and generally stick to only that subset of JSDoc that is understood by TypeScript (see write-up on TypeScript website here: "JSDoc Reference").

And my "Future" item 2:

Trivial to configure a NPM script or build step that uses the TypeScript compiler to generate Type Declaration files (see here) and expose them alongside the build artifacts.

So, I was also thinking along the sames lines about what the future could potentially look like.

My suggestion would be to embrace the TypeScript-style comment annotations, prioritize TS IDE/LSP compatibility, and perhaps going the next step of adding TypeScript as a dev dependency to generate type definitions. It's not strictly necessary or required by this change, but I think it's good benefits with few drawbacks.

However, that may not be the direction the library maintainers want to go, and I defer to their preference.

@03juan
Copy link
Contributor

03juan commented Oct 30, 2023

@tgroshon yes absolutely, I just wanted to point out the specific issues to the maintainers as well and of course defer to their vision. Your initial comment was very thorough and well reasoned, I should have referenced that and just emphasised your points.

I'd aso like to see the direction of this leaning towards TS, even though I don't use it personally, simply because IDE tooling is supporting it by default these days. Unless one is hand-rolling their neovim configuration or something, but then those people know what they need to do anyways. 🤣

@@ -223,6 +223,9 @@ like this:
Then a hook callback object could be defined and passed to the socket:

```javascript
/**
* @type {Object.<string, import("phoenix_live_view").ViewHook>}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this render correctly when generating LV docs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is a valid JS code.

Copy link
Author

@tgroshon tgroshon Oct 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Object.<string, ViewHook> is the Google Closure syntax which is a valid syntax in JSDoc. In typescript it would be {[key: string]: ViewHook}. Both syntaxes are understood by JSDoc and TypeScript. I'm not sure what any JSDoc annotation will do within Hex Docs.

@srcrip
Copy link
Contributor

srcrip commented Nov 21, 2023

This is an amazingly good change. JSDoc is a huge step up from relying on DefinitelyTyped. It's extremely hard to keep DefinitelyTyped up to date in a library that's changing as often as Live View does. What is remaining to be done here? Is there anything I can help with? How soon could this be merged?

@tgroshon
Copy link
Author

@sevensidedmarble I think it's just waiting on review/comments and a merge.

@tgroshon
Copy link
Author

tgroshon commented Dec 5, 2023

@sevensidedmarble If you want a cool project, you should check out rendered.js and try to get a good type annotation for the over-the-wire protocol. Could be a good Advent of Code problem ;)

I'd really like to correctly annotate and declare the type definition in renderer.js for the over-the-wire live view protocol for communicating rendered diff trees. I spent a few hours trying to reverse engineer it from (a) code, (b) tests, and (c) this fantastic article by Jose', but... I couldn't quite nail it down, so I used a very loose type. This would probably be easy for someone more familiar with problem domain and implementation.

@chrismccord
Copy link
Member

I'm honestly not sure about this PR from a maintenance perspective. I can see the benefits, but I'm not yet convinced they'll be enough of a help for us. Can you open a new PR that docs only the public interfaces? (So the ViewHook and public interface of LiveSocket ?). That I indeed seeing being helpful for folks, but I don't think the exhaustive docs on the private interfaces is necessarily what we want. Thank you!

@tgroshon
Copy link
Author

tgroshon commented Jan 23, 2024

Thanks for taking the time to take a look at this @chrismccord! That's very reasonable a position, and I support you. The linked PR by the great @RodolfoSilva should accomplish that. If you ever want to investigate going further with typing interfaces, don't hesitate to reach out.

All the best!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants