The Wayback Machine - https://web.archive.org/web/20200530123339/https://github.com/facebook/react/issues/17885
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

Enable a lint rule not to define after return and fix existing callsites #17885

Open
sebmarkbage opened this issue Jan 21, 2020 · 5 comments
Open

Comments

@sebmarkbage
Copy link
Member

@sebmarkbage sebmarkbage commented Jan 21, 2020

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

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

function getPooledWarningPropertyDefinition(propName, getVal) {

It's also just a confusing pattern at best.

@M-Izadmehr
Copy link
Contributor

@M-Izadmehr M-Izadmehr commented Jan 21, 2020

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

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Jan 21, 2020

@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
Copy link
Member Author

@sebmarkbage sebmarkbage commented Jan 21, 2020

@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
Copy link
Contributor

@M-Izadmehr M-Izadmehr commented Feb 3, 2020

@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.

image

@flexhard69
Copy link

@flexhard69 flexhard69 commented Apr 26, 2020

Bind

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.