[New] jsx-max-depth: validate a specific depth for JSX
#1260
Conversation
There was a problem hiding this comment.
Can we also add test cases for nested inline JSX?
In other words, with a limit of 2, <div>{<div><div><span /></div></div>}</div> should still error on the inner piece - also, ideally, with a limit of 2, <div>{<div><span /></div>}</div> should also error because the total is 3, even tho it's never more than 2 in a chunk.
Similarly, const x = <div><span /><div>; <div>{x}</div> would be nice to fail also.
docs/rules/jsx-max-depth.md
Outdated
|
|
||
| ```js | ||
| ... | ||
| "react/jsx-no-depth": [<enabled>, <number>] |
There was a problem hiding this comment.
let's make an initial schema that's extensible; in other words, { "max": <number> } instead of just the number.
lib/rules/jsx-max-depth.js
Outdated
|
|
||
| var MESSAGE = 'JSX Element are nested too deeply ({{depth}}).'; | ||
|
|
||
| var maxDepth = context.options[0] || 3; |
There was a problem hiding this comment.
in new code, let's use const/let over var
lib/rules/jsx-max-depth.js
Outdated
|
|
||
| create: function(context) { | ||
|
|
||
| var MESSAGE = 'JSX Element are nested too deeply ({{depth}}).'; |
There was a problem hiding this comment.
let's also include the current max length in the message, like max-len does.
tests/lib/rules/jsx-max-depth.js
Outdated
| var RuleTester = require('eslint').RuleTester; | ||
|
|
||
| var parserOptions = { | ||
| ecmaVersion: 8, |
There was a problem hiding this comment.
let's set this to 6, since we shouldn't need ES2017 for this rule
tests/lib/rules/jsx-max-depth.js
Outdated
| ecmaVersion: 8, | ||
| sourceType: 'module', | ||
| ecmaFeatures: { | ||
| experimentalObjectRestSpread: true, |
There was a problem hiding this comment.
similarly, this feature isn't required here
chriswong
force-pushed
the
max-depth
branch
2 times, most recently
from
0132b8a
to
3f91f7a
Compare
lib/rules/jsx-max-depth.js
Outdated
| schema: [{ | ||
| oneOf: [ | ||
| { | ||
| type: 'integer', |
There was a problem hiding this comment.
I'd prefer to not even allow this as an option, if possible - meaning, to only have the object schema.
lib/rules/jsx-max-depth.js
Outdated
| } | ||
|
|
||
| function findJSXElement(variables, name) { | ||
| let find = function(refs) { |
There was a problem hiding this comment.
const - or a function declaration
There was a problem hiding this comment.
Done. Changed to function declaration.
lib/rules/jsx-max-depth.js
Outdated
| let i = refs.length; | ||
|
|
||
| while (--i >= 0) { | ||
| if ('writeExpr' in refs[i]) { |
There was a problem hiding this comment.
please use the has package rather than in, to avoid traversing the prototype chain.
lib/rules/jsx-max-depth.js
Outdated
|
|
||
| while (--i >= 0) { | ||
| if ('writeExpr' in refs[i]) { | ||
| let writeExpr = refs[i].writeExpr; |
lib/rules/jsx-max-depth.js
Outdated
| let writeExpr = refs[i].writeExpr; | ||
|
|
||
| return isJSXElement(writeExpr) | ||
| && writeExpr |
There was a problem hiding this comment.
these chained lines should be indented one level further in
lib/rules/jsx-max-depth.js
Outdated
| let element = findJSXElement(variables, node.expression.name); | ||
|
|
||
| if (element) { | ||
| let baseDepth = getDepth(node); |
lib/rules/jsx-max-depth.js
Outdated
| } | ||
|
|
||
| function isLeaf(node) { | ||
| let children = node.children; |
lib/rules/jsx-max-depth.js
Outdated
| const DEFAULT_DEPTH = 3; | ||
|
|
||
| let option = context.options[0] || DEFAULT_DEPTH; | ||
| let maxDepth = (typeof option === 'number' ? option : option.max) || DEFAULT_DEPTH; |
There was a problem hiding this comment.
const
Also, what happens if I pass a value of 0? This looks like it will always set 0 to 3. Please add some tests for values of 0 - ie, no nesting allowed.
There was a problem hiding this comment.
max: {
type: 'integer',
minimum: 1
}0 is no a valid value now.
There was a problem hiding this comment.
Why isn't zero a valid value? I'd think prohibiting nesting entirely is a totally valid option.
lib/rules/jsx-max-depth.js
Outdated
| const MESSAGE = 'Expected the depth of JSX Elements nested should be {{needed}} but found {{gotten}}.'; | ||
| const DEFAULT_DEPTH = 3; | ||
|
|
||
| let option = context.options[0] || DEFAULT_DEPTH; |
lib/rules/jsx-max-depth.js
Outdated
| }] | ||
| }, | ||
| create: function(context) { | ||
| const MESSAGE = 'Expected the depth of JSX Elements nested should be {{needed}} but found {{gotten}}.'; |
There was a problem hiding this comment.
"gotten" seems like an odd word choice; maybe "found"?
lib/rules/jsx-max-depth.js
Outdated
| properties: { | ||
| max: { | ||
| type: 'integer', | ||
| minimum: 1 |
There was a problem hiding this comment.
i think this should be made to work with 0.
There was a problem hiding this comment.
Since this rule is called "max-depth", I think it should detect the "depth" but not "nesting depth" of jsx, a depth of 0 means "no element here" which is absolutely useless
There was a problem hiding this comment.
If that's true, then a max of 1 would permit <div /> but forbid <div><span /><div>; is that what the current PR's docs and test cases indicate?
There was a problem hiding this comment.
Specifically, I want it to be configurable so I can forbid any nesting whatsoever - a depth of 0 means "flat", so in fact <div /> has a depth of zero.
|
Is there anything stopping this rule from moving forward? This seems pretty useful to me! 😄 It probably needs a rebase though, at least one file has a conflict. |
|
Happy to rereview after a rebase. |
|
@chriswong any chance you could rebase this? |
|
@ljharb I rebased this on master. How should I submit the changes? |
|
Certainly not with a duplicate PR, which I can never delete from my git history :-( Posting a link to the branch would be more than sufficient. |
|
I've rebased this properly and pushed the changes up to here and to #1676. |
Fixes #1219