ESLint: jsx-max-depth

[grahamlangford]

What does this PR do?

• Last rule in Enable more React rules eslint-config-pixiebrix#194

Discussion

• Opening to encourage discussion about the rule
• See https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/jsx-max-depth.md
• Discouraging deeply nested jsx makes components easier to read
• Anything deeper than 5 gets very difficult to reason about
• We have a lot of violations that would need to be cleaned up, might make sense to set this rule to warn?

Future Work

• Once rule is implemented completely, migrate to eslint-config-pixiebrix

Checklist

• Designate a primary reviewer

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.54%. Comparing base (93c88b6) to head (bf527c7).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7819      +/-   ##
==========================================
- Coverage   72.56%   72.54%   -0.03%     
==========================================
  Files        1274     1274              
  Lines       39834    39834              
  Branches     7399     7399              
==========================================
- Hits        28905    28897       -8     
- Misses      10929    10937       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[BLoe]

I would be open to this, but I think there are exceptions where nesting deeply makes the code more readable, such as top level App components that need to nest a lot of Providers/Wrappers/etc. I wouldn't want to force ourselves to arbitrarily split those up into multiple stacks of nested 1-line component calls, so we'd just need to suppress this lint rule at those places.

[grahamlangford]

I would be open to this, but I think there are exceptions where nesting deeply makes the code more readable, such as top level App components that need to nest a lot of Providers/Wrappers/etc. I wouldn't want to force ourselves to arbitrarily split those up into multiple stacks of nested 1-line component calls, so we'd just need to suppress this lint rule at those places.

This is fair. But I would suggest that those situations would be so rare we could readily eslint-ignore those.

[fregante]

I agree with Ben. This rule only makes sense if we view it as an opportunity to break up larger components into smaller ones, however I'm not entirely convinced:

• that this won't just be ignored
• that it offers a meaningful solution to this at 5 levels

I do like some sort of hard limit on excessive code though, but only when it's very obviously "bad" and hard to follow: "10K LOC files", "1K LOC functions", "20 nested levels".

The complexity rule is similar to this and still has a few suppressions in the codebase, even as a warning. I have low hopes for this at 5 levels.

[grahamlangford]

I agree with Ben. This rule only makes sense if we view it as an opportunity to break up larger components into smaller ones, however I'm not entirely convinced:

• that this won't just be ignored
• that it offers a meaningful solution to this at 5 levels

I do like some sort of hard limit on excessive code though, but only when it's very obviously "bad" and hard to follow: "10K LOC files", "1K LOC functions", "20 nested levels".

The complexity rule is similar to this and still has a few suppressions in the codebase, even as a warning. I have low hopes for this at 5 levels.

Here's an example of one of the violations. Breaking this up into smaller components would dramatically increase clarity.

pixiebrix-extension/src/components/brickModalNoTags/BrickModal.tsx

Lines 230 to 315 in 5df7760

	return (
	<Modal
	className={cx(styles.root, modalClassName)}
	show
	centered
	size="xl"
	onHide={close}
	backdrop
	keyboard={false}
	>
	<Modal.Body className={styles.body}>
	<Container fluid>
	<Row>
	<Col xs={5} className={styles.results}>
	<Form>
	<InputGroup>
	<InputGroup.Prepend>
	<InputGroup.Text>Search</InputGroup.Text>
	</InputGroup.Prepend>
	<Form.Control
	ref={searchInput}
	placeholder="Start typing to find results"
	value={query}
	onChange={({ target }) => {
	setQuery(target.value);
	}}
	/>
	</InputGroup>
	</Form>
	<div>
	<AutoSizer>
	{({ height, width }: Size) => (
	<LazyList
	height={height}
	width={width}
	itemCount={searchResults.length}
	itemSize={brickResultSizePx}
	itemKey={itemKey}
	itemData={
	{
	searchResults,
	setDetailBrick,
	activeBrick: detailBrick,
	selectCaption,
	onSelect,
	close,
	} as ItemType<T>
	}
	>
	{ItemRenderer}
	</LazyList>
	)}
	</AutoSizer>
	</div>
	</Col>
	<Col
	xs={7}
	className={cx(styles.brickDetail)}
	key={detailBrick?.id}
	>
	{detailBrick ? (
	<BrickDetail
	brick={detailBrick}
	listing={listings[detailBrick.id]}
	selectCaption={selectCaption}
	onSelect={() => {
	onSelect(detailBrick);
	close();
	}}
	/>
	) : (
	<QuickAdd
	onSelect={(brick) => {
	onSelect(brick);
	close();
	}}
	recommendations={recommendedBricks}
	/>
	)}
	</Col>
	</Row>
	</Container>
	</Modal.Body>
	</Modal>
	);
	}

Number of levels is something I'm definitely willing to have a discussion on. I honestly picked 5 more or less at random. Just a "feels kinda right" amount.

If we were to merge the rule, I would argue to keep it on "error" and actually fix everything. We should also look at actually enforcing our complexity rule finally in a separate DevEx ticket.

[twschiller]

Closing in favor of: #7865