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

docs: add TSDoc comments interface-internal module #2949

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Khwahish29
Copy link
Contributor

@Khwahish29 Khwahish29 commented Feb 9, 2025

Title

docs: Add TSDoc comments to @libp2p/interface-internal (#2113 )

Description

This PR adds TSDoc comments to @libp2p/interface-internal, ensuring that all exported interfaces, types, and constants have proper documentation. This improves the generated API documentation and aligns with the project's documentation standards.

Notes & open questions

  • Please review if the added examples align with best practices.
  • Let me know if any additional clarifications are needed.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

@Khwahish29 Khwahish29 requested a review from a team as a code owner February 9, 2025 10:28
@Khwahish29 Khwahish29 changed the title Add TSDoc comments to @libp2p/interface-internal docs: Add TSDoc comments to @libp2p/interface-internal Feb 9, 2025
@maschad maschad changed the title docs: Add TSDoc comments to @libp2p/interface-internal docs: add TSDoc comments interface-internal module Feb 10, 2025
Copy link
Member

@maschad maschad left a comment

Choose a reason for hiding this comment

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

Great work here @Khwahish29 . I've left some comments.

My personal opinion is that we don't need examples for the applicability these internal interfaces, but interested to see @achingbrain 's opinion.

packages/interface-internal/src/connection-manager.ts Outdated Show resolved Hide resolved
packages/interface-internal/src/connection-manager.ts Outdated Show resolved Hide resolved
packages/interface-internal/src/random-walk.ts Outdated Show resolved Hide resolved
packages/interface-internal/src/transport-manager.ts Outdated Show resolved Hide resolved
@Khwahish29
Copy link
Contributor Author

Great work here @Khwahish29 . I've left some comments.

My personal opinion is that we don't need examples for the applicability these internal interfaces, but interested to see @achingbrain 's opinion.

Hey @maschad! Took your feedback and updated the TSDoc comments for @libp2p/interface-internal

  • Removed unnecessary examples.
  • Cleaned up descriptions to keep things clear and consistent.

Really appreciate the thoughtful feedback! Let me know if there’s anything else to tweak. 🚀

@@ -1,5 +1,13 @@
import type { Multiaddr } from '@multiformats/multiaddr'

/**
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need @packageDocumentation tags for each file here.

From the docs:

Used to indicate a doc comment that describes an entire NPM package (as opposed to an individual API item belonging to that package)

The text from the @packageDocumentation tags you've added could be used in the tag describing the main interface exported from each file instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification! 🙌
I’ve moved the @packageDocumentation tags. Let me know if this looks good or if there’s anything else to adjust!

Copy link
Member

@maschad maschad left a comment

Choose a reason for hiding this comment

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

Thanks for this @Khwahish29 this LGTM

@maschad maschad requested a review from achingbrain February 12, 2025 16:40
Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

Good stuff, comments inline.

I would definitely recommend running npm run docs and checking the generated output in the .docs folder as I think some of the changes here won't necessarily appear where you think they will (details are below).

Comment on lines 60 to 69
/**
* The `AddressManager` module provides an interface for managing peer addresses
* in libp2p. It supports handling multiple types of addresses, verifying their validity,
* and storing mappings between internal and external addresses.
*/
/**
* Interface for managing peer addresses in libp2p.
*/

export interface AddressManager {
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be one comment, not two, otherwise only the second will appear in the generated docs.

You can check this by running npm run docs in the root of the repo and looking at the HTML that appears in the .docs folder.

Suggested change
/**
* The `AddressManager` module provides an interface for managing peer addresses
* in libp2p. It supports handling multiple types of addresses, verifying their validity,
* and storing mappings between internal and external addresses.
*/
/**
* Interface for managing peer addresses in libp2p.
*/
export interface AddressManager {
/**
* The `AddressManager` provides an interface for managing peer addresses
* in libp2p. It supports handling multiple types of addresses, verifying their validity,
* and storing mappings between internal and external addresses.
*/
export interface AddressManager {

Comment on lines 40 to 48
/**
* The `ConnectionManager` module handles managing connections between peers in a libp2p network.
* It provides methods for opening, closing, and querying connections.This also provides methods
* for accessing the dial queue.
*/
/**
* Manages the dialing, opening, and closing of connections.
*/

Copy link
Member

Choose a reason for hiding this comment

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

Must be a single comment, not two:

Suggested change
/**
* The `ConnectionManager` module handles managing connections between peers in a libp2p network.
* It provides methods for opening, closing, and querying connections.This also provides methods
* for accessing the dial queue.
*/
/**
* Manages the dialing, opening, and closing of connections.
*/
/**
* The `ConnectionManager` handles managing connections between peers in a libp2p network.
* It provides methods for opening, closing, and querying connections.This also provides methods
* for accessing the dial queue.
*/

Comment on lines 4 to 11
* The `RandomWalk` module facilitates peer discovery by randomly finding and dialing peers
* on the libp2p network. It is useful in conjunction with a registered `Topology` to
* discover common network services.
*/

/**
* The `RandomWalk` interface provides a mechanism for discovering and connecting to
* random peers on the libp2p network.
Copy link
Member

Choose a reason for hiding this comment

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

Must be a single comment, not two.

Also, the docs were wrong here - RandomWalk only discovers peers, it doesn't dial them.

Suggested change
* The `RandomWalk` module facilitates peer discovery by randomly finding and dialing peers
* on the libp2p network. It is useful in conjunction with a registered `Topology` to
* discover common network services.
*/
/**
* The `RandomWalk` interface provides a mechanism for discovering and connecting to
* random peers on the libp2p network.
* The `RandomWalk` component uses the libp2p peer routing to find arbitrary
* network peers. Consumers may then dial these peers, causing the Identify
* protocol to run and any registered topologies to be notified of their
* supported protocols.

@@ -1,5 +1,10 @@
import type { StreamHandler, StreamHandlerOptions, StreamHandlerRecord, Topology, IncomingStreamData } from '@libp2p/interface'

/**
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this comment is necessary - these are named exports so will get their own documentation blocks.

Please can you check the output of npm run docs (look in the .docs folder) and delete this comment if it doesn't appear anywhere.

Comment on lines 29 to 38
/**
* The `Registrar` module provides an interface for managing protocol handlers
* and topologies in a libp2p network. It enables registering and managing
* protocol-specific handlers, ensuring efficient peer-to-peer communication.
*/
/**
* The `Registrar` interface allows modules to register, manage, and remove
* protocol handlers and topology discovery mechanisms in libp2p.
*/

Copy link
Member

Choose a reason for hiding this comment

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

Must be a single comment, not two.

Also we can be a bit more specific in the comment:

Suggested change
/**
* The `Registrar` module provides an interface for managing protocol handlers
* and topologies in a libp2p network. It enables registering and managing
* protocol-specific handlers, ensuring efficient peer-to-peer communication.
*/
/**
* The `Registrar` interface allows modules to register, manage, and remove
* protocol handlers and topology discovery mechanisms in libp2p.
*/
/**
* The `Registrar` provides an interface for registering protocol handlers -
* these are invoked when remote peers open streams on the local node with the
* corresponding protocol name.
*
* It also allows configuring network topologies for a given protocol(s). The
* topology callbacks are invoked when a peer that supports those protocols
* connects or disconnects.
*
* The Identify protocol must be configured on the current node for topologies
* to function.
*/

*
* An id will be returned that can later be used to unregister the
Copy link
Member

Choose a reason for hiding this comment

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

This part about the id being used to unregister the topology later is important, please do not delete it.

Comment on lines 11 to 19
/**
* The `TransportManager` module handles the management of transport protocols in a libp2p network.
* It is responsible for managing the transport themselves - dialling, querying addresses and listening.
*/

/**
* The `TransportManager` interface manages available transports for dialing and listening in a libp2p node.
*/

Copy link
Member

Choose a reason for hiding this comment

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

Must be a single comment, not two.

Also protocols is a loaded word here, since it is used to refer to protocol streams. Better to just call transports "transports" or "network transports" and not "transport protocols".

Also, also, it's probably worth noting that consumers should generally prefer to open connections using the connection manager.

Suggested change
/**
* The `TransportManager` module handles the management of transport protocols in a libp2p network.
* It is responsible for managing the transport themselves - dialling, querying addresses and listening.
*/
/**
* The `TransportManager` interface manages available transports for dialing and listening in a libp2p node.
*/
/**
* The `TransportManager` handles the management of network transports, allowing
* opening connections or listening using specific transports, etc.
*
* This is a low-level component - any connections opened will not be managed by
* the `ConnectionManager` or included in any configured connection limits, etc.
*
* Most consumers will call `openConnection` on the `ConnectionManager` instead.
*/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@achingbrain I've addressed the latest round of feedback—please let me know if there's anything else that needs further improvement.
I truly appreciate the time and effort you’ve put into reviewing my work and ensuring it meets the right standards :)

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

Successfully merging this pull request may close these issues.

3 participants