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

no-use-before-define needs exception option for arrow function definitions present in common code patterns #12986

Closed
ildarmgt opened this issue Feb 29, 2020 · 3 comments · Fixed by #13017
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion documentation Relates to ESLint's documentation rule Relates to ESLint's core rules

Comments

@ildarmgt
Copy link

ildarmgt commented Feb 29, 2020

What rule do you want to change?

no-use-before-define

Does this change cause the rule to produce more or fewer warnings?

For those using new exception, same or fewer

How will the change be implemented? (New option, new default behavior, etc.)?

New option to ignore rule for arrow functions

Please provide some example code that this change will affect:

// typical React type module or large helper library
// with many functions using each other in definitions but not evaluated
// until parsed when exported functions get imported and afterwards executed

// most important functions first like react components or main interfaces
export const Main = () => {
  const renderMath = getC(getB(getA()))
  return renderMath
}

// 100s of interconnected helpers, not easy to order
export const getA = (v = 0) => getB() + getC(v)
const getB = (v) => v || 3
const getC = (v) => v || getA(1)

// here or after export/import
document.body.innerHTML = Main()

https://codesandbox.io/s/nervous-pond-7x5xo

What does the rule currently do for this code?

Treats unevaluated arrow functions same as evaluated variables. eslint.org/demo...

While reasons given are to protect from getting value from undefined variables, they prevent forming connections in the prototyping declaration design phase before any instances are created while intending to aid with evaluation of instances.

Description of no-use-before-define suggests ReferenceError will be thrown with any attempt to access the variable which is not the case for variables that are not evaluated when they or the block scope they are in is not called to be evaluated. The rule also discusses use prior to es6 as risk, but hoisted functions are almost never used, es6 arrows are the standard, and importing a multiple pre-declared functions from same location (equivalent to declaring definitions before evaluation) is extremely common. Rule provides optional exceptions for classes and functions which helped transition, but not for arrow functions. Classes, in fact, have exception option despite not being hoisted, but arrow functions do not in a way that doesn't overlap with actual variables. React, for example, is almost entirely es6 functional now and depends heavily on declarations.

It prevents using modern es6 syntax where functions are declared using each other, forcing often impossible sorting. It also forces the least important functions to the top. Many projects have completely switched to es6 syntax for consistency, refuse to use old function syntax & its this scope. So they do not have any option in es-lint to keep definition checks for variables easily without refactoring working code and risking circular dependencies or breaking syntax consistency.

This syntax is extremely common today in mass exporting modules where many individual functions are named exports and another file imports via `import { a, b, c, d } from './somelibrary' that might reference each other's definitions when they shared scope in declaration file, only evaluating/executing long after all definitions are parsed on import. Similarly when reading React modules, reader should have an option not to have to scroll through hundreds of small irrelevant helper function declarations not worth separating into 100s of individual modules to find to main component declaration. Helpers might be as simple as getters, setters, wrappers, styling using each other in prototypes. The old function(){} or class formats and designs are being abandoned in favor of functional code and pure arrow functions so old exclusions for classes and classic functions are not helpful.

This is not an edge case of only recursive functions but connections between modules in general. This is for scope of all function declarations, and not just let/const, including within anon params or objects { t : () => getT(), getT }. Anon arrow function declarations are often used specifically to NOT evaluate inaccessible items at declaration time until ready .

We have to choose to either :

  1. Turn off the most important linter rule of checking declarations for all variables that actually do create errors because they are evaluated at same time as declared.

  2. Have to abandon modern syntax for arbitrary functions, change this in their scope, make them harder to find by regex shape. For short anon function callbacks have to switch to much wordier notation.

Another argument is that eslint isn't supposed to fix users js, only let people follow some standards, but without this exception, it can break pretty standard use of arrow functions, and there's no easy rule for common patterns that both watches actual variable declarations (most typical helpful lint warning) and allows common and popular functional syntax.

What will the rule do after it's changed?

No changes, just new option to add to very similar existing ones:

Simple solution: new option to avoid triggering warning/error for arrow functions. For example: arrowFunctions: boolean as new option. That's what it did for similar non-hoisted classes & variables. Should have same for functions, which are now arrow functions. Will be enough for me.

Are you willing to submit a pull request to implement this change?

Not at the moment as unfamiliar with code base

Thank you!

@ildarmgt ildarmgt added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules triage An ESLint team member will look at this issue soon labels Feb 29, 2020
@mdjermanovic
Copy link
Member

Hi @ildarmgt, thanks for the issue and the detailed description of the problem.

I think this is already covered by "variables": false, which means that the rule will ignore all references from functions to variables in outer scopes:

/*eslint no-use-before-define: ["error", { "variables": false }]*/

b(); // error

const a = () => b(); // ok

const b = () => {};

b(); // ok

Online Demo

With these settings, the example you provided would be lint-free:

/*eslint no-use-before-define: ["error", { "variables": false }]*/

export const Main = () => {
  const renderMath = getC(getB(getA()))
  return renderMath
}

// 100s of interconnected helpers, not easy to order
export const getA = (v = 0) => getB() + getC(v)
const getB = (v) => v || 3
const getC = (v) => v || getA(1)

Online Demo

@ildarmgt
Copy link
Author

ildarmgt commented Mar 2, 2020

wow you're totally right, it does appear to have that capability!

demo (2nd one would give an error but tough example)

That's great. The rule description did not make it as clear parser understands the difference. The variable example was a direct value link and it should probably mention it includes variables as functions.

I saw this come up a lot while searching and people usually trying the functions and/or nofunc rule - it's really strange classes go into nofunc but not arrow functions. People often treat variables linked to functions, links by references, and links by value different.

e.g.

#9861 (comment)

My suggestion is to simply just mention variables as functions or arrow functions in variables exception paragraph, would've been enough for me.

Thank you x 100

@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion documentation Relates to ESLint's documentation and removed enhancement This change enhances an existing feature of ESLint triage An ESLint team member will look at this issue soon labels Mar 2, 2020
@mdjermanovic
Copy link
Member

I saw this come up a lot while searching and people usually trying the functions and/or nofunc rule - it's really strange classes go into nofunc but not arrow functions. People often treat variables linked to functions, links by references, and links by value different.

nofunc sets just "functions": false, not "classes".

I agree, "functionDeclarations" instead of "functions" would probably avoid confusion about the meaning of this option (though, the documentation does state that the option applies to function declarations only).

My suggestion is to simply just mention variables as functions or arrow functions in variables exception paragraph, would've been enough for me.

Agreed! It would be nice to add some examples with function expressions and arrow functions, in the variables section.

Marking this as an accepted issue to improve documentation, PR is welcome!

kaicataldo added a commit that referenced this issue Mar 23, 2020
…#13017)

* Docs: added fn decl example and details for variables option

* Docs: added more example for variables options

* Chore: refactore variables example

* Chore: removed extra EOF line

* Chore: update default for nofunc no-use-before-define

Co-Authored-By: Kai Cataldo <kai@kaicataldo.com>

Co-authored-by: Kai Cataldo <kai@kaicataldo.com>
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Sep 21, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Sep 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion documentation Relates to ESLint's documentation rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants