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

[explicit-module-boundary-types] Warns on constructor overload #2150

Closed
TheBrokenRail opened this issue Jun 2, 2020 · 9 comments · Fixed by #2158
Closed

[explicit-module-boundary-types] Warns on constructor overload #2150

TheBrokenRail opened this issue Jun 2, 2020 · 9 comments · Fixed by #2158
Labels
bug Something isn't working has pr there is a PR raised to close this package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@TheBrokenRail
Copy link

TheBrokenRail commented Jun 2, 2020

Repro

export class Test {
    constructor();
    constrcutor(value?: string) {
        console.log(value);
    }
}

Expected Result
No warnings

Actual Result

114:16  warning  Missing return type on function  @typescript-eslint/explicit-module-boundary-types

Versions

package version
@typescript-eslint/eslint-plugin 3.1.0
@typescript-eslint/parser 3.1.0
TypeScript 3.9.3
ESLint 7.1.0
node 14.2.0
npm 6.14.4
@TheBrokenRail TheBrokenRail added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Jun 2, 2020
@bradzacher
Copy link
Member

Warning on lines 2

Could you please elaborate?

Also this code shouldn't ever match the rule, as it should only inspect exported things - could you please provide a full repro?

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for team members to take a look labels Jun 2, 2020
@SkeLLLa
Copy link

SkeLLLa commented Jun 2, 2020

I've got this rule warning even without overloading constructor.

declare namespace MyClass {
    interface Options {
      // ...
    }
}

declare class MyClass {
    constructor(options?: MyClass.Options); // here Missing return type on function  @typescript-eslint/explicit-module-boundary-types
}

export = MyClass;

Eslint config:

const tsPlugin = require('@typescript-eslint/eslint-plugin');
module.exports = {
    overrides: [
        {
            files: ['**/*.ts'],
            plugins: ['@typescript-eslint'],
            rules: {
                ...tsPlugin.configs['eslint-recommended'].overrides.rules,
                ...tsPlugin.configs.recommended.rules,
                'no-redeclare': 'off',
                '@typescript-eslint/ban-types': 'off',
                '@typescript-eslint/require-await': 'off',
            },
        },
    ],
};

@TheBrokenRail
Copy link
Author

Updates reproduction!

@chunghha
Copy link

chunghha commented Jun 2, 2020

To me, this warning appears with not only with constructor but also from a function which, both cases, I don't see with v3.0.2. To reproduce, please check out the develop branch of https://github.com/chunghha/docker-ts-nest/tree/develop You will see the warning for former pic below by "yarn lint" with v3.1.0 but won't with v3.0.2 regardless of the eslint-disable-next-line comment there.

image

image

@bradzacher bradzacher added bug Something isn't working has pr there is a PR raised to close this and removed awaiting response Issues waiting for a reply from the OP or another party labels Jun 2, 2020
@bradzacher
Copy link
Member

bradzacher commented Jun 2, 2020

@chunghha - this is expected and intentional.
You have parameters on those functions that are not typed.

	public rate<T>(
		@Param('from', new UpperCasePipe()) from: string,
		@Param('to', new UpperCasePipe()) to: string,
		@Res() response
//             ^^^^^^^^ ERROR - THIS PARAMETER IS UNTYPED
	): void {

    constructor(
      @Inject(CONTEXT) private readonly ctx,
//                     ^^^^^^^^^^^^^^^^^^^^ ERROR - THIS PARAMETER IS UNTYPED
      @Inject('winston') private readonly logger: Logger,
      private readonly metadataService: MetadataService
    ) {}

In the latest release I reworked the error locations so that they highlight the arguments that are untyped; so your suppression comments are no longer suppressing the errors.

@chunghha
Copy link

chunghha commented Jun 2, 2020

@bradzacher - the comment is intentional to suppress the warning because I am aware of some of parameters are hard to be typed. And, prettier does formatting of the lines including line breaks. Therefore, I prefer the behavior of v3.0.2 than v3.1.0 if you refer to this location. But, the fix in v3.1.0 is permanent way forward, I'll then adjust the comment location in my code .
image

@bradzacher
Copy link
Member

bradzacher commented Jun 2, 2020

This is intentional and will be the way forward.
Instead of highlighting the entire function when there is an untyped argument, it's much clearer to just highlighting just the untyped argument. This makes it easier to find and fix the problem.

You should not leave untyped arguments in the codebase. Untyped arguments are simply typed as any, which removes all type safety from the function body, as well as from the function callsites.

@TheBrokenRail
Copy link
Author

In my issue all the arguments are typed, and it only highlights the overloads.

@TheBrokenRail
Copy link
Author

It specifically mentions missing the return type.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working has pr there is a PR raised to close this package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
4 participants