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

Eslint plugin: prefer-box-no-classname rule w/ autofix + merged into prefer-box-lonely-ref #1629

Merged
merged 6 commits into from
Aug 19, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 8 additions & 7 deletions docs/src/Eslint Plugin.doc.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,23 +32,24 @@ card(
`}
/>
<MainSection.Subsection
title="gestalt/prefer-box"
title="gestalt/prefer-box-inline-style"
description={`
Prevent using \`<div>\` inline styling for attributes that are already implemented in Box.

[Learn more about Box](/Box).
`}
/>
<MainSection.Subsection
title="gestalt/prefer-box-lonely-ref"
description={`
Prevent \`<div>\` tags used to only contain a \`ref\` attribute.
title="gestalt/prefer-box-no-disallowed"
description={`Prevent \`<div>\` tags that don't contain disallowed attributes: className and onClick. Use Gestalt Box, instead. Other attributes are disallowed as well so this Eslint rule doesn't conflict with [eslint-plugin-jsx-a11y](https://github.com/jsx-eslint/eslint-plugin-jsx-a11y).

Instead, use \`ref\` props supported in Box or other elements such as Button or TextField.
[Read more about Box](/Box).

With AUTOFIX!
It also prevents \`<div>\` tags used to only contain a \`ref\` attribute. \`ref\` is supported in Box and other elements such as Button or TextField.

[Read more about the ref prop in Box](/Box#Using-as-a-ref).

[Read more about the ref prop in Box](/Box#Using-as-a-ref).
With AUTOFIX!
`}
/>
<MainSection.Subsection
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@ import { Box as RenamedBox } from 'gestalt';
export default function TestElement() {
const ref = useRef(null);
return (
<Box>
<RenamedBox>
<div ref={ref}>
<Box />
<RenamedBox />
</div>
</Box>
</RenamedBox>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@ import { Box as RenamedBox } from 'gestalt';
export default function TestElement() {
const ref = useRef(null);
return (
<Box>
<Box ref={ref}>
<Box />
</Box>
</Box>
<RenamedBox>
<RenamedBox ref={ref}>
<RenamedBox />
</RenamedBox>
</RenamedBox>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ export default function TestElement() {
const ref = useRef(null);
return (
<div ref={ref}>
<div />
<button />
</div>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ export default function TestElement() {
const ref = useRef(null);
return (
<Box ref={ref}>
<div />
<button />
</Box>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { Box } from 'gestalt';
export default function TestElement() {
return (
<Box>
<div onBlur={() => {}} style={{ marginTop: 200 }}>
<Box />
</div>
</Box>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { Box } from 'gestalt';
export default function TestElement() {
return (
<Box>
<Box onBlur={() => {}} dangerouslySetInlineStyle={{ __style: { marginTop: 200 } }}>
<Box />
</Box>
</Box>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@ import { Box } from 'gestalt';
export default function TestElement() {
return (
<Box>
<Box ref={undefined} />
<div ref={undefined} className={undefined} />
<div onBlur={() => {}}>
<Box />
</div>
</Box>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { Box } from 'gestalt';
export default function TestElement() {
return (
<Box>
<Box onBlur={() => {}}>
<Box />
</Box>
</Box>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { Box as RenamedBox } from 'gestalt';
export default function TestElement() {
return (
<RenamedBox>
<div onBlur={() => {}}>
<RenamedBox />
</div>
</RenamedBox>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { Box as RenamedBox } from 'gestalt';
export default function TestElement() {
return (
<RenamedBox>
<RenamedBox onBlur={() => {}}>
<RenamedBox />
</RenamedBox>
</RenamedBox>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { Text, Flex } from 'gestalt';
export default function TestElement() {
return (
<div onBlur={() => {}}>
<Text>Test</Text>
</div>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { Box, Flex, Text } from 'gestalt';
export default function TestElement() {
return (
<Box onBlur={() => {}}>
<Text>Test</Text>
</Box>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
export default function TestElement() {
return (
<div onBlur={() => {}}>
<button />
</div>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { Box } from 'gestalt';
export default function TestElement() {
return (
<Box onBlur={() => {}}>
<button />
</Box>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function TestElement() {
return <div onBlur={() => {}} />;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import { Box } from 'gestalt';
export default function TestElement() {
return <Box onBlur={() => {}} />;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { Box } from 'gestalt';
export default function TestElement() {
const props = { onBlur: () => {} };
return (
<Box>
<Box ref={undefined} />
<div ref={undefined} className={undefined} />
<div ref={undefined} onClick={() => {}} />
<div ref={undefined} tabIndex={-1} />
<div ref={undefined} role="button" />
<div ref={undefined} onMouseOver={() => {}} />
<div ref={undefined} accessKey="test" />
<div ref={undefined} autoFocus />
<div ref={undefined} {...props} />
</Box>
);
}
45 changes: 35 additions & 10 deletions packages/eslint-plugin-gestalt/src/eslintASTFixers.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
// @flow strict
import { getNamedImportsComponents, getTextNodeFromSourceCode } from './eslintASTHelpers.js';
import {
getLocalComponentImportName,
getNamedImportsComponents,
getTextNodeFromSourceCode,
} from './eslintASTHelpers.js';

// $FlowFixMe[unclear-type]
type GenericNode = {| [string]: any |};
Expand Down Expand Up @@ -56,7 +60,9 @@ type RenameTagFixerType = ({|
context: GenericNode,
elementNode: GenericNode,
fixer: GenericNode,
gestaltImportNode: GenericNode,
newComponentName: string,
replaceRegexCallback?: ({| input: string |}) => string,
tagName: string,
|}) => $ReadOnlyArray<GenericNode>;

Expand All @@ -70,42 +76,55 @@ export const renameTagFixer: RenameTagFixerType = ({
context,
elementNode,
fixer,
gestaltImportNode,
newComponentName,
replaceRegexCallback = ({ input }) => input,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a bit specific to dangerouslySetInlineStyle. I wonder if it would make more sense to build that prop (and any others) as a string outside of this function, then use RenameTagWithPropsFixerType's additionalPropsString arg to build it into the fixed component?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the less "invasive" approach, I'm not just renaming, but adding the { __style: <>} first key value . Adding one additional props would require removing the previous style prop. I could also reconstruct all props and alphabetize, but that would make the PR review even more opaque to the important changes. I might change the approach when I work on other Eslints, there must be a more universal way. For now, I think I wanna keep this one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of the approach I took with the Box->Flex rule?

Avoiding regex keeps the code more readable, and it seems a lot simpler to me to keep the rule-specific logic outside of these helpers. The helpers can remove a current prop or add a new one, and the logic within the rule can determine what needs to be removed and what needs to be added (and how the new prop(s) should be formatted).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DONE

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Looks great. This arg can now be removed, right?

tagName,
}) => {
return [elementNode.openingElement, elementNode.closingElement]
.map((node) => {
// $FlowFixMe[incompatible-type] Flow is not detecting the method filter(Boolean)
if (!node) return false;
const namedImportsComponents =
getNamedImportsComponents({
importNode: gestaltImportNode,
}) ?? [];

return fixer.replaceText(
node,
getTextNodeFromSourceCode({ context, elementNode: node }).replace(
const componentNameMatch = namedImportsComponents.find(
(item) => item[0] === newComponentName,
);

const replacedText = replaceRegexCallback({
input: getTextNodeFromSourceCode({ context, elementNode: node }).replace(
tagName,
newComponentName,
(componentNameMatch && componentNameMatch[1]) ?? newComponentName,
),
);
});
return fixer.replaceText(node, replacedText);
})
.filter(Boolean);
};

type RenameTagWithPropsFixerType = ({|
additionalPropsString: string,
fixedPropsString: string,
context: GenericNode,
elementNode: GenericNode,
fixer: GenericNode,
gestaltImportNode: GenericNode,
newComponentName: string,
tagName: string,
propsToRemove?: $ReadOnlyArray<string>,
|}) => $ReadOnlyArray<GenericNode>;

/** This function is a more complex version of renameTagFixer. It has the same tag replacement functionality, but it also rebuild the props in the opening tag to include a new prop. The new prop must be formatted as a string, p.e. `as="article"`
Examples 1:
"\<div\>\<\/div\>" if tagName="div", newComponentName="Box", and completePropsString=`as="article"` returns "\<Box as="article"\>\<\/Box\>"
*/
export const renameTagWithPropsFixer: RenameTagWithPropsFixerType = ({
additionalPropsString,
fixedPropsString,
context,
elementNode,
gestaltImportNode,
fixer,
newComponentName,
tagName,
Expand All @@ -115,16 +134,22 @@ export const renameTagWithPropsFixer: RenameTagWithPropsFixerType = ({
// $FlowFixMe[incompatible-type] Flow is not detecting the method filter(Boolean)
if (!node) return false;

const completeOpeningNode = `<${newComponentName} ${additionalPropsString}${
const finalNewComponentName = getLocalComponentImportName({
importNode: gestaltImportNode,
componentName: newComponentName,
});

const completeOpeningNode = `<${finalNewComponentName} ${fixedPropsString}${
elementNode.closingElement ? '' : ' /'
}>`;

return index === 0
? fixer.replaceText(node, completeOpeningNode)
: fixer.replaceText(
node,
getTextNodeFromSourceCode({ context, elementNode: node }).replace(
tagName,
newComponentName,
finalNewComponentName,
),
);
})
Expand Down