Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Improve typedef for callback used as parameter #4533

Merged
merged 1 commit into from
Oct 5, 2019
Merged

Improve typedef for callback used as parameter #4533

merged 1 commit into from
Oct 5, 2019

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Feb 23, 2019

PR checklist

Overview of change:

Typedef arrow-call-signature now return error for

someFunc(() => {});

Is there anything you'd like reviewers to focus on?

Actually, with all typedef rules actives.

const A = () => {} // error (return type)
someFunc(() => {}); // no error
someFunc((n) => {}); // error (param type)
setTimeout(function() {}, 1000); // error (return type)

After the PR, error everywhere ; for consistency.

Here #2000 (comment), a suggestion is a child-arrow-call-signature. That's a possibility, but we have to (for consistency) add a child-arrow-parameter too, which is also a suggestion here #813.
We, then, have to decide before if we want also a child-parameter and a child-call-signature.

I can do it if you prefer.
Personally I don't think all these options are necessary. The typedef is a style rule for people who want to type the code (more like a documentation) even if it's not necessary thanks to inference. If you don't want to be redundant and infer as much type you can, just use the noImplicitAny option in tsconfig.

CHANGELOG.md entry:

[enhancement] typedef rule arrow-call-signature option is more consistent in reporting errors on lambdas and will flag more violations that were missed in the previous rule implementation.

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

The code looks good to me (nice clean implementation!), but since this is a breaking change, I think we'd need to wait until 6.0 to merge in.

/cc @adidahiya

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Feb 24, 2019

@JoshuaKGoldberg
The question "Is it breaking change or is it bug fix" is in a way kinda debatable.

There was no consistency in this example

const A = () => {} // error (return type)
someFunc(() => {}); // no error
someFunc((n) => {}); // error (param type)
setTimeout(function() {}, 1000); // error (return type)

So you can think it was a bug to not detect the error for callback return type.

@adidahiya adidahiya self-assigned this Feb 25, 2019
@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Mar 4, 2019

@JoshuaKGoldberg @adidahiya Is it a breaking change or a bug fix ? I think it's debatable.

When is the v6 supposed to be released ?
If it's in a long time, an option can do the work while waiting

@JoshuaKGoldberg
Copy link
Contributor

breaking change or a bug fix

Both? 😄

Even if this is considered a bug, it's a pretty big change. If a project already enables the rule under the expectation that scenario of arrow lambdas passed to methods shouldn't be flagged, then it starts flagging them, it'd become more difficult to migrate to the next TSLint version (even if this is desirable).

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

yes this is a breaking change and if you want to merge this it needs to be under a new option. I suggest naming the option check-call-signatures

setTimeout(function() {}, 1000);

const foo = n => n+1
~~~~ [expected arrow-call-signature to have a typedef]
Copy link
Contributor

Choose a reason for hiding this comment

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

why does the failure range extend to the =? that seems a little off

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In const foo = n => n+1 location.pos = 11
in const foo = (n => n+1) location.pos = 13

I didn't touch this part

someFunc(n => n+1);
~~~ [expected arrow-call-signature to have a typedef]
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, feels strange to include the ( in the failure range

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's made for (n) => n+1
.....................~~~ [expected arrow-call-signature to have a typedef]

The Node is only n.
But the code use this.addFailure(location.pos - 1, location.end + 1, failure);
I didn't touch this part

@VincentLanglet
Copy link
Contributor Author

@JoshuaKGoldberg @adidahiya I agree it will be easier with something like arrow-callback-call-signature option. But it does not make any sense having an option for callback in arrow function, and not in standard function, or an option for callback return type in arrow function, and not parameter type in callback function.

A bug fix is always a breaking change. Before the rule had false positive, now it correctly find the errors.

But if you prefer, you can wait for the next major version. But in this case I have a question. When is it planned ? There is already a lot of Breaking changes waiting for it.
https://github.com/palantir/tslint/pulls?q=is%3Aopen+is%3Apr+label%3A%22Type%3A+Breaking+Change%22

@JoshuaKGoldberg
Copy link
Contributor

@VincentLanglet great question! 😄 just filed #4811 to discuss.

@VincentLanglet
Copy link
Contributor Author

@JoshuaKGoldberg Great ! Is this PR ready for 6.0.0 version ?

@JoshuaKGoldberg
Copy link
Contributor

Deferring to @adidahiya for review now that we're ready for breaking changes (#4811).

@adidahiya
Copy link
Contributor

thanks for the PR @VincentLanglet!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants