Skip to content

fix: event declaration additional tags #253

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

Merged

Conversation

tr2-harada
Copy link
Contributor

Regarding the comment for the event declaration, it seems that the @deprecated tag description does not follow JSDoc rules because it is written for multiple methods at once.
https://jsdoc.app/tags-deprecated.html
https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html#deprecated
typescript-eslint/typescript-eslint#1223

With the current output format, some methods are not detected by ESLint (eslint-plugin-deprecation) error checking, so it may be difficult to notice when an event you are using is deprecated.
I think it would be better to be able to emit the @deprecated tag for each method so that ESLint can check all usage of deprecated events.

Before correction:

/**
  * Emitted when the renderer process crashes or is killed.
  *
  * **Deprecated:** This event is superceded by the `render-process-gone` event
  * which contains more information about why the render process disappeared.
  * isn't always because it crashed. The `killed` boolean can be replaced by
  * checking `reason === 'killed'` when you switch to that event.
  *
  * @deprecated
  */
on(event: 'crashed', listener: (event: Event,
                                 killed: boolean) => void): this;
off(event: 'crashed', listener: (event: Event,
                                 killed: boolean) => void): this;
once(event: 'crashed', listener: (event: Event,
                                 killed: boolean) => void): this;
addListener(event: 'crashed', listener: (event: Event,
                                 killed: boolean) => void): this;
removeListener(event: 'crashed', listener: (event: Event,
                                 killed: boolean) => void): this;

Revised:

/**
  * Emitted when the renderer process crashes or is killed.
  *
  * **Deprecated:** This event is superceded by the `render-process-gone` event
  * which contains more information about why the render process disappeared.
  * isn't always because it crashed. The `killed` boolean can be replaced by
  * checking `reason === 'killed'` when you switch to that event.
  *
  * @deprecated
  */
on(event: 'crashed', listener: (event: Event,
                                 killed: boolean) => void): this;
/**
  * @deprecated
  */
off(event: 'crashed', listener: (event: Event,
                                 killed: boolean) => void): this;
/**
  * @deprecated
  */
once(event: 'crashed', listener: (event: Event,
                                 killed: boolean) => void): this;
/**
  * @deprecated
  */
addListener(event: 'crashed', listener: (event: Event,
                                 killed: boolean) => void): this;
/**
  * @deprecated
  */
removeListener(event: 'crashed', listener: (event: Event,
                                 killed: boolean) => void): this;

@tr2-harada tr2-harada requested review from a team as code owners November 28, 2023 04:35
Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

@tr2-harada would you be willing to add a test for this change?

@tr2-harada
Copy link
Contributor Author

@codebytere
Thank you for your review.
Added a test for creating tag-only comments in utils.wrapComment.
Please let me know if I misunderstood your intent.

Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

Thanks @tr2-harada! Appreciated 🙇🏻‍♀️

@codebytere codebytere merged commit c48a231 into electron:main Nov 29, 2023
Copy link

🎉 This PR is included in version 8.15.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@tr2-harada tr2-harada deleted the fix-event-declaration-additional-tags branch January 9, 2024 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants