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

comment: write js docs in on #5198

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

Conversation

nayounsang
Copy link
Contributor

@nayounsang nayounsang commented Sep 25, 2024

The kind of change this PR does introduce

  • a bug fix
  • a new feature
  • an update to the documentation
  • a code change that improves performance
  • other

Current behavior

  • About socket-io-client-emitter/Emitter on method.
  • It was difficult for us to know what the reserved event names were without looking at the docs.

New behavior

image

  • I thought it would be better to use JSDoc without changing the types. This is because there is an advantage of auto-completion when using union, etc., but developers often use custom event names, so I thought the type could actually cause confusion.
  • So I used JSDoc. If the user checks the JSDoc, they can see the scheduled event name and its brief description.
    If developers are curious about more, I have linked the docs.

Other information (e.g. related issues)

  • If you want to edit, change or delete content, please always leave a comment.
  • I've only written about on at the moment, so if you think it would be good to write about something else as well, please let me know.

@darrachequesne
Copy link
Member

Hi! Thanks a lot for your PR.

I'm not sure this is right though, as the emitter provided by the @socket.io/component-emitter is used in several places, not just for the Socket object of the socket.io-client package.

Another possibility would be to override the on method in the Socket class, and add the improved documentation there. What do you think?

@nayounsang
Copy link
Contributor Author

@darrachequesne
There is a way like that! I also think this is better.

@nayounsang
Copy link
Contributor Author

nayounsang commented Oct 23, 2024

Hi. I commit this.
Is this correct what you said?

  • I was confused about where to place the overridden method. First of all, since it is a new change, I placed it at the very bottom.
  • I am wondering how to use the ReversedEvents type defined as a generic in the Emitter. Basically, it simply inherits EventMaps, so I created a new interface: interface ReservedEvents extends EventsMap {}. However, there remains a question as to whether this will ensure a consistent type with the on of the Emiiter in the future.

If it doesn't fit your intention, or if you have any comments, please let me know!

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.

2 participants