Skip to content

fix(eslint-plugin): [explicit-function-return-type] Fix allowExpressions ignoring default exports #831

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Aug 10, 2019
Merged

fix(eslint-plugin): [explicit-function-return-type] Fix allowExpressions ignoring default exports #831

merged 4 commits into from
Aug 10, 2019

Conversation

Svish
Copy link
Contributor

@Svish Svish commented Aug 9, 2019 •
edited
Loading

The current implementation of allowExpressions below causes default exports to be allowed/ignored:

if (
  options.allowExpressions &&
  node.parent.type !== AST_NODE_TYPES.VariableDeclarator &&
  node.parent.type !== AST_NODE_TYPES.MethodDefinition
) {
  return;
}

I really think a default export should be considered as much a declaration/definition as a variable declaration is. Maybe even more so, as it's the main export of a module.

Currently only the last one of these fail when allowExpressions is set to true:

export default () => {};     // <-- Allowed
export const fn = () => {};  // <-- Not allowed

So in this PR I've added AST_NODE_TYPES.ExportDefaultDeclaration to the check (and tests for it), so that both those cases are correctly (in my opinion) rejected as lacking a return type.

export default () => {};     // <-- No longer allowed
export const fn = () => {};  // <-- Still not allowed

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @Svish!

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

@Svish
Copy link
Contributor Author

Svish commented Aug 9, 2019

By the way, I don't know if I should also create an issue for this issue? That's what I considered to do to begin with, but since I realized it would be super easy to fix, I decided to just create a PR directly instead. But yeah... not sure what you want for project tracking purposes and such. 🤔

@bradzacher bradzacher added breaking change This change will require a new major version to be released enhancement New feature or request labels Aug 9, 2019
@bradzacher bradzacher added this to the 2.0.0 milestone Aug 9, 2019
bradzacher
bradzacher requested changes Aug 9, 2019
View reviewed changes
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

I agree with you, and I'm happy with this change.
It's breaking, but we're going to release 2.0 soon, so we can include this.

The code LGTM.

One change before this can be merged; could you please update the rule doc appropriately.

@bradzacher bradzacher added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Aug 9, 2019
@Svish
Copy link
Contributor Author

Svish commented Aug 9, 2019

@bradzacher Cool! About updating the rule doc, I definitely could, but looking at it now, I'm not sure exactly what I should update. What it says there now is already correct.

if true, only functions which are part of a declaration will be checked

The check I've added is ExportDefaultDeclaration, so it's covered by "part of a declaration". I suppose I could add my 1 valid and 2 invalid tests to the rule doc examples... Want me to do that? And are there other adjustments you think should be made? 🤔

@Svish
Copy link
Contributor Author

Svish commented Aug 9, 2019

I've added two examples of code that after this change will still be incorrect, even with allowExpressions set to true. Not sure what else would make sense to add/change though...

As a minor cleanup of my own earlier PR, I also simplified the examples for allowHigherOrderFunctions by removing the function parameters. Looking at them again now, I realized they aren't actually relevant to the examples at all, so they are really just adding clutter.

bradzacher
bradzacher approved these changes Aug 10, 2019
View reviewed changes
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

Yeah that's perfect. I just wanted to be sure that the export default case was included in the readme.

LGTM - thanks for doing this.

@bradzacher bradzacher added bug Something isn't working and removed 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge enhancement New feature or request labels Aug 10, 2019
@bradzacher bradzacher merged commit ebbcc01 into typescript-eslint:master Aug 10, 2019
@bradzacher bradzacher mentioned this pull request Aug 10, 2019
Closed
14 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bradzacher bradzacher bradzacher approved these changes

Assignees
No one assigned
Labels
breaking change This change will require a new major version to be released bug Something isn't working
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@Svish @bradzacher