React: Enable a lint rule not to define after return and fix existing callsites

[sebmarkbage]

https://twitter.com/therealyashsriv/status/1219691914523545601

We shouldn't generate code that might cause browser or linting to complain.

https://github.com/facebook/react/blob/0cf22a56a18790ef34c71bef14f64695c0498619/packages/legacy-events/SyntheticEvent.js#L259

It's also just a confusing pattern at best.

[M-Izadmehr]

Hey,
If possible I would like to take a look into this issue:)

[bvaughn]

@M-Izadmehr This issue is all yours! 😄

We've added the "good first issue (taken)" label so that others will know not to start work on the issue. If you change your mind about the issue, no worries! Just let us know so that we can remove the label and free it up for someone else to claim.

Cheers!

[sebmarkbage]

@M-Izadmehr Great! Thanks for taking a look. I think the first step is to look at our es-lint config and enable a canonical lint rule that already exists upstream.

After that you should be able to open a PR which should fail the lint rule. After that you could do a separate commit that contains the code fixes. I wouldn't start by fixing the code though as that risks rebase issues.

[M-Izadmehr]

@sebmarkbage @bvaughn
I tried to check this issue and here are my findings:
First of all, we are never violating no-unreachable linting rule.
Based on MDN firefox never shows a warning, if a function declaration statement is following return. Also, based on ESLint it is also a valid code. As a result react code is safe, and never causes a warning.

Secondly, after a small search through the project, I see that we are using this approach in at least a dozen places. I can suggest two things, if we are on the same page, then continue with the PR:

1. Explicitly add no-unreachable to eslint config (this is already a part of default config, so adding it will have no effect, just helps with explicitly showing the purpose)
2. Although this pattern does not cause a warning (in either eslint/or browser), it can be confusing at times (however, it also results in cleaner functions, e.g. check below image)

Either way, if you agree with refactoring such cases I can move on with submitting the PR.

[flexhard69]

Bind

[bpernick]

@M-Izadmehr @bvaughn Hello all! is this still being worked on?

[mnindrazaka]

Hi, everyone, is there any update about this issue ?
@bvaughn @M-Izadmehr @sebmarkbage

[bhumijgupta]

Hey @bvaughn @sebmarkbage , I recently created an ES-Lint plugin (eslint-plugin-no-function-declare-after-return) to ensure there are no function declaration after return. I can :-

1. Add the plugin to the repo
   or
2. Extract logic out of the plugin and add a new rule to the repo
   and then make changes to fix it.

P.S. I added the plugin to the local fork of the repo and tested it. It was able to detect 6 places in codebase where there is a function declaration after return.

[bvaughn]

Extract logic out of the plugin and add a new rule to the repo

I'm not sure what "extract logic out of the plugin" mean. Is this option just adding the eslint-plugin-no-function-declare-after-return plugin to our .eslintrc.js? If so, sounds reasonable to me.

[bhumijgupta]

Yup, I can just add the plugin to .eslintrc.js and it will work just fine.
If this looks good, can you assign the issue to me, so I can start working on the PR?

[bvaughn]

Great. Send a PR and I'll take a look :)

[bvaughn]

Thanks @bhumijgupta

[bhumijgupta]

Looking for more opportunities to contribute

[bvaughn]

I see 452 open issues at the moment 😅 Maybe there's another one you find interesting