docs: modify examples for explicit-module-boundary-types #7404
Conversation
|
Thanks for the PR, @Ryan-Dia! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Ryan-Dia
changed the title
There was a problem hiding this comment.
The point of this rule is:
- Exported members must have explicit signatures
- Non-exported members may or may not have explicit signatures
Your change makes everything exported, so the second case is lost. I'm not sure how this is an improvement. If you wish, though, we can have both an exported and a non-exported example.
When I first encountered these two points in the rule, having both of them in a single example made it harder to understand. Therefore, as you suggested, instead of putting both points in one example, dividing them into separate examples would make it much more intuitive, clear, and easier to comprehend. As you mentioned, I created two separate examples for each of the two points you made. Now, what do you think would be the best way to modify them? |
There was a problem hiding this comment.
Per https://deploy-preview-7404--typescript-eslint.netlify.app/contributing/pull-requests, we ask that each sent PR either addresses an open issue or is for an "extremely minor" documentation typos. This isn't minor. Could you file an issue please? I think we'd want to discuss this a bit more.
packages/eslint-plugin/docs/rules/explicit-module-boundary-types.md
Outdated
Show resolved
Hide resolved
JoshuaKGoldberg
added
the
awaiting response
At first, I thought it was a minor matter, but as we talked, it seemed that there were more significant aspects to consider. As you mentioned, I have filed the issue. Thank you. Issues : #7430 |
JoshuaKGoldberg
removed
the
awaiting response
| @@ -12,9 +12,11 @@ It can also improve TypeScript type checking performance on larger codebases. | |||
|
|
|||
| ## Examples | |||
|
|
|||
| ### 1. Exported members must have explicit signatures | |||
|
|
|||
| <!--tabs--> | |||
There was a problem hiding this comment.
Looking at this first pair of tabs:
- ✏️
export function test() {: Great! - ✏️
export default function () {: Odd that the Correct version is different. I'd say it should probably instead have an explicit return type. export var arrowFn: Great!- ✏️ I wonder if we can delete number 2 (
export default function() {), since thisarrowFnalso serves the purpose of showing different syntax?
- ✏️ I wonder if we can delete number 2 (
export var arrowFn = (arg):: Great!export var arrowFn = (arg: any):: Great!- ✏️
export class Test {: Mostly great, but can you move the Correct code's comment to just abovemethod()? It's confusing IMO to see it change location (and the new location is inaccurate to what the comment says)
✏️ indicates a request for changes :)
| return; | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ### 2. Non-exported members may or may not have explicit signatures | ||
|
|
||
| <!--tabs--> |
There was a problem hiding this comment.
I also find it confusing how this second pair of tabs has such different code between Incorrect and Correct. And all it's trying to convey is that an alternative to adding an explicit type annotation is to not export a function.
Maybe instead of having a whole second tabs pair, a single mention could be made in the first tabs pair? I think that'd be easier to read & would result in a more streamlined docs page.
JoshuaKGoldberg
added
the
awaiting response
All of these requests have been reflected. 😁
I agree.
// A function with no return value (void)
export function test(): void {
return;
}
// A return value of type string
export var arrowFn = (): string => 'test';
// All arguments should be typed
export var arrowFn = (arg: string): string => `test ${arg}`;
export var arrowFn = (arg: unknown): string => `test ${arg}`;
export class Test {
// A class method with no return value (void)
method(): void {
return;
}
}
// The function does not apply because it is not an exported function.
function test() {
return;
}
// If you remove the export, you have the option to either write explicit return and argument types or not.
// A function with no return value (void)
export function test(): void {
return;
}
// A return value of type string
export var arrowFn = (): string => 'test';
// All arguments should be typed
export var arrowFn = (arg: string): string => `test ${arg}`;
export var arrowFn = (arg: unknown): string => `test ${arg}`;
export class Test {
// A class method with no return value (void)
method(): void {
return;
}
}
// case 1: A function with no return value (void)
export function test(): void {
return;
}
// case 2: Function is not exported
function test() {
return;
}
// case 1: A return value of type string
export var arrowFn = (): string => 'test';
// case 2: arrowFn is not exported
var arrowFn = (arg): string => `test ${arg}`;
// case 1: All arguments should be typed
export var arrowFn = (arg: string): string => `test ${arg}`;
export var arrowFn = (arg: unknown): string => `test ${arg}`;
// case 2: arrowFn is not exported
var arrowFn = (arg): string => `test ${arg}`;
var arrowFn = (arg: any): string => `test ${arg}`;
export class Test {
// case 1: A class method with no return value (void)
method(): void {
return;
}
}
// case 2: Class is not exported
class Test {
method() {
return;
}
} |
JoshuaKGoldberg
removed
the
awaiting response
|
Err, I think case 1? It's been a few weeks and there's a lot of text to read through 😅 sorry. |
JoshuaKGoldberg
added
the
awaiting response
That's all right. They said it was confusing because it was divided into two tabs before, so I would like to write it as one tab. I prepared 3 cases of styles. Currently, case 3 style is applied to PR. Does your response mean that Case 1 style is good? |
|
Let's go with case 1 and then we can review from there 👍 |
|
👋 Hey @Ryan-Dia! Just checking in, is this still something you have time for? No worries if not - I just don't want to leave it hanging. |
JoshuaKGoldberg
added
the
stale
I'm sorry for the delay. I applied it to case 1 you mentioned last time. |
JoshuaKGoldberg
removed
awaiting response
…eslint#7404) * docs : modify examples for explicit-module-boundary-types * docs : update examples for explicit-module-boundary-types * docs: fix atx-style * docs: update code based on the request * docs: fix typo * docs: change case 3 to case 1
PR Checklist
Overview
I had difficulty understanding the
explicit-module-boundary-typesrule when I first encountered it due to unclear and non-intuitive examples.In the initial description of this rule, it states, "Require explicit return and argument types on exported functions and classes public class methods." When writing code that violates this rule in the playground, it shows an error message like "Missing return type on function," as seen in the image above. Furthermore, in the third image, the comment for Incorrect code reads, "Should indicate that no value is returned (void)."
Based on this information, one way to address this issue is to avoid exporting meaningless functions or classes. However, to better understand and adhere to this rule in a clear and intuitive manner, I believe it is more appropriate to follow the approach described in the document: explicitly specifying types to avoid errors. By doing so, we can easily grasp the essence of the rule, which emphasizes "Requiring explicit return and argument types on exported functions and classes public class methods", creating more straightforward and intuitive examples.
Reference
https://typescript-eslint.io/rules/explicit-module-boundary-types/