[New] `jsx-max-depth`: validate a specific depth for JSX #1260

chriswong · 2017-06-14T13:29:36Z

ljharb

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.

ljharb · 2017-06-14T19:02:04Z

docs/rules/jsx-max-depth.md

+
+```js
+...
+"react/jsx-no-depth": [<enabled>, <number>]


let's make an initial schema that's extensible; in other words, { "max": <number> } instead of just the number.

ljharb · 2017-06-14T19:02:17Z

lib/rules/jsx-max-depth.js

+
+    var MESSAGE = 'JSX Element are nested too deeply ({{depth}}).';
+
+    var maxDepth = context.options[0] || 3;


in new code, let's use const/let over var

ljharb · 2017-06-14T19:02:48Z

lib/rules/jsx-max-depth.js

+
+  create: function(context) {
+
+    var MESSAGE = 'JSX Element are nested too deeply ({{depth}}).';


let's also include the current max length in the message, like max-len does.

ljharb · 2017-06-14T19:03:09Z

tests/lib/rules/jsx-max-depth.js

+var RuleTester = require('eslint').RuleTester;
+
+var parserOptions = {
+  ecmaVersion: 8,


let's set this to 6, since we shouldn't need ES2017 for this rule

ljharb · 2017-06-14T19:03:17Z

tests/lib/rules/jsx-max-depth.js

+  ecmaVersion: 8,
+  sourceType: 'module',
+  ecmaFeatures: {
+    experimentalObjectRestSpread: true,


similarly, this feature isn't required here

ljharb

Thanks, this is looking much better.

ljharb · 2017-06-21T18:13:32Z

lib/rules/jsx-max-depth.js

+    schema: [{
+      oneOf: [
+        {
+          type: 'integer',


I'd prefer to not even allow this as an option, if possible - meaning, to only have the object schema.

ljharb · 2017-06-21T18:15:10Z

lib/rules/jsx-max-depth.js

+    }
+
+    function findJSXElement(variables, name) {
+      let find = function(refs) {


const - or a function declaration

Done. Changed to function declaration.

ljharb · 2017-06-21T18:15:35Z

lib/rules/jsx-max-depth.js

+        let i = refs.length;
+
+        while (--i >= 0) {
+          if ('writeExpr' in refs[i]) {


please use the has package rather than in, to avoid traversing the prototype chain.

ljharb · 2017-06-21T18:15:40Z

lib/rules/jsx-max-depth.js

+
+        while (--i >= 0) {
+          if ('writeExpr' in refs[i]) {
+            let writeExpr = refs[i].writeExpr;


ljharb · 2017-06-21T18:15:50Z

lib/rules/jsx-max-depth.js

+            let writeExpr = refs[i].writeExpr;
+
+            return isJSXElement(writeExpr)
+            && writeExpr


these chained lines should be indented one level further in

ljharb · 2017-06-21T18:16:22Z

lib/rules/jsx-max-depth.js

+        let element = findJSXElement(variables, node.expression.name);
+
+        if (element) {
+          let baseDepth = getDepth(node);


ljharb · 2017-06-21T18:16:41Z

lib/rules/jsx-max-depth.js

+    }
+
+    function isLeaf(node) {
+      let children = node.children;


ljharb · 2017-06-21T18:17:19Z

lib/rules/jsx-max-depth.js

+    const DEFAULT_DEPTH = 3;
+
+    let option = context.options[0] || DEFAULT_DEPTH;
+    let maxDepth = (typeof option === 'number' ? option : option.max) || DEFAULT_DEPTH;


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.

max: { type: 'integer', minimum: 1 }

0 is no a valid value now.

Why isn't zero a valid value? I'd think prohibiting nesting entirely is a totally valid option.

ljharb · 2017-06-21T18:17:24Z

lib/rules/jsx-max-depth.js

+    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;


ljharb · 2017-06-21T18:17:47Z

lib/rules/jsx-max-depth.js

+    }]
+  },
+  create: function(context) {
+    const MESSAGE = 'Expected the depth of JSX Elements nested should be {{needed}} but found {{gotten}}.';


"gotten" seems like an odd word choice; maybe "found"?

ljharb · 2017-06-22T04:21:43Z

lib/rules/jsx-max-depth.js

+        properties: {
+          max: {
+            type: 'integer',
+            minimum: 1


i think this should be made to work with 0.

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

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?

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.

jseminck · 2017-08-31T08:23:04Z

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.

ljharb · 2017-08-31T17:36:57Z

Happy to rereview after a rebase.

jseminck · 2017-11-16T20:17:29Z

@chriswong any chance you could rebase this?

forabi · 2018-02-04T16:19:32Z

@ljharb I rebased this on master. How should I submit the changes?

ljharb · 2018-02-04T17:18:55Z

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.

ljharb · 2018-02-04T17:22:15Z

I've rebased this properly and pushed the changes up to here and to #1676.

[New] Add jsx-max-depth

34f83a5

Fixes jsx-eslint#1219.

ljharb requested changes Jun 14, 2017

View reviewed changes

ljharb added the new rule label Jun 14, 2017

chriswong force-pushed the max-depth branch 2 times, most recently from 0132b8a to 3f91f7a Compare June 21, 2017 14:16

ljharb requested changes Jun 21, 2017

View reviewed changes

chriswong force-pushed the max-depth branch from 3f91f7a to 89cc617 Compare June 22, 2017 04:08

ljharb reviewed Jun 22, 2017

View reviewed changes

chriswong force-pushed the max-depth branch from 89cc617 to 4724c6a Compare June 22, 2017 06:53

yannickcr force-pushed the master branch from f4737f0 to 73abadb Compare August 9, 2017 21:48

yannickcr force-pushed the master branch from c8dedeb to dc9f85e Compare September 24, 2017 18:16

yannickcr force-pushed the master branch from dca5cd5 to c148893 Compare November 18, 2017 17:26

wholesomedev mentioned this pull request Jan 13, 2018

Launch new Tom Hanks page hollowverse-archive/hollowverse#230

Merged

7 tasks

forabi mentioned this pull request Jan 21, 2018

Lint against deeply nested code hollowverse-archive/hollowverse#252

Closed

forabi mentioned this pull request Feb 4, 2018

[Rebased] jsx-max-depth: validate a specific depth for JSX #1676

Merged

ljharb force-pushed the max-depth branch from 4724c6a to 48562dd Compare February 4, 2018 17:21

ljharb force-pushed the max-depth branch from 48562dd to f2cb60d Compare February 4, 2018 17:36

ljharb approved these changes Feb 4, 2018

View reviewed changes

ljharb requested review from yannickcr, lencioni and EvHaus February 4, 2018 17:46

ljharb force-pushed the max-depth branch from f2cb60d to 34f83a5 Compare February 19, 2018 22:42

ljharb merged commit 34f83a5 into jsx-eslint:master Feb 19, 2018

renovate bot mentioned this pull request Mar 22, 2018

Update eslint-plugin-react to 7.8.2 sbardian/DebtTrackerAPI#26

Merged

renovate bot mentioned this pull request Sep 8, 2024

Update all non-major dependencies element-hq/compound-web#231

Merged

1 task

This was referenced Sep 27, 2024

feat: add shouldCacheResult option sanity-io/hydrogen-sanity#96

Merged

chore(deps): update all non-major dependencies UniversityOfHelsinkiCS/palaute#1310

Merged

renovate bot mentioned this pull request Oct 26, 2024

chore(deps): update all devdependencies apollographql/apollo-client-devtools#1412

Open

1 task


		var MESSAGE = 'JSX Element are nested too deeply ({{depth}}).';

		var maxDepth = context.options[0] \|\| 3;


		create: function(context) {

		var MESSAGE = 'JSX Element are nested too deeply ({{depth}}).';

[New] jsx-max-depth: validate a specific depth for JSX #1260

[New] jsx-max-depth: validate a specific depth for JSX #1260

Conversation

chriswong commented Jun 14, 2017

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jseminck commented Aug 31, 2017

ljharb commented Aug 31, 2017

jseminck commented Nov 16, 2017

forabi commented Feb 4, 2018

ljharb commented Feb 4, 2018

ljharb commented Feb 4, 2018

[New] `jsx-max-depth`: validate a specific depth for JSX #1260

[New] `jsx-max-depth`: validate a specific depth for JSX #1260