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

fix(rw-eslint): Implement more specific checking on Routes #10404

Merged
merged 7 commits into from
Apr 3, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changesets/10404.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
- fix(rw-eslint): Implement more specific checking on Routes (#10404) by @dac09

Fixes: If you use any other elements outside the Route tree it should not throw a linting error or warning
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,32 @@ ruleTester.run('unsupported-route-components', unsupportedRouteComponents, {
)
}`.replace(/ +/g, ' '),
},
{
code: `
const AnotherThing = <Bazinga><p>Hello</p></Bazinga>
const Routes = () => {
return (
<Router>
<PrivateSet
whileLoadingAuth={AnotherThing}
>
Tobbe marked this conversation as resolved.
Show resolved Hide resolved
<Route path="/contacts" page={ContactsPage} name="contacts" />
</PrivateSet>
</Router>
)
}`.replace(/ +/g, ' '),
},
],
invalid: [
{
code: 'const Routes = () => <Router><div><Route path="/" page={HomePage} name="home" /></div></Router>',
errors: [{ messageId: 'unexpected' }],
},
// block statement style
{
code: 'const Routes = () => { return (<Router><div><Route path="/" page={HomePage} name="home" /></div></Router>) }',
errors: [{ messageId: 'unexpected' }],
},
{
code: `
const Routes = () => {
Expand Down
66 changes: 52 additions & 14 deletions packages/eslint-plugin/src/unsupported-route-components.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import type { TSESTree } from '@typescript-eslint/utils'
import { ESLintUtils } from '@typescript-eslint/utils'

const createRule = ESLintUtils.RuleCreator.withoutDocs
Expand All @@ -7,6 +8,29 @@ function isAllowedElement(name: string) {
return allowedElements.includes(name)
}

function checkNodes(
nodesToCheck: TSESTree.JSXElement | TSESTree.JSXChild,
context: any,
dac09 marked this conversation as resolved.
Show resolved Hide resolved
) {
if (nodesToCheck.type === 'JSXElement') {
const name =
nodesToCheck.openingElement.name.type === 'JSXIdentifier'
? nodesToCheck.openingElement.name.name
: null
if (name && !isAllowedElement(name)) {
context.report({
node: nodesToCheck,
messageId: 'unexpected',
data: { name },
})
}

if (nodesToCheck.children) {
nodesToCheck.children.forEach((node) => checkNodes(node, context))
}
}
}

export const unsupportedRouteComponents = createRule({
meta: {
type: 'problem',
Expand All @@ -18,28 +42,42 @@ export const unsupportedRouteComponents = createRule({
unexpected:
'Unexpected JSX element <{{name}}>. Only <Router>, <Route>, <Set>, <PrivateSet> and <Private> are allowed in the Routes file.',
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to change the error message to be more specific too?

Copy link
Contributor Author

@dac09 dac09 Apr 3, 2024

Choose a reason for hiding this comment

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

No I don't think so, because the problem was that it was targetting all JSX Elements. The behaviour actually hasn't changed, just stops incorrectly targetting irrelavnt elements

Copy link
Member

@Tobbe Tobbe Apr 3, 2024

Choose a reason for hiding this comment

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

Only <Router>, <Route>, <Set>, <PrivateSet> and <Private> are allowed in the Routes file.

It's not true though, is it? You can have const AnotherThing = <Bazinga><p>Hello</p></Bazinga> in your Routes file and it's still perfectly valid, right? We just want to warn against doing crazy things inside the <Router> element itself, not the entire file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see what you're saying!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

},
schema: [], // No additional configuration needed
schema: [],
},
defaultOptions: [],
create(context) {
return {
JSXOpeningElement: function (node) {
let name = ''
VariableDeclaration(node) {
if (isRoutesRenderBlock(node.declarations[0])) {
const routesDeclaration = node.declarations[0].init

if (node.name.type === 'JSXIdentifier') {
name = node.name.name
} else {
return
}
if (routesDeclaration?.type === 'ArrowFunctionExpression') {
if (routesDeclaration.body.type === 'JSXElement') {
// Routes = () => <Router>...</Router>
checkNodes(routesDeclaration.body, context)
} else if (routesDeclaration.body.type === 'BlockStatement') {
// For when Routes = () => { return (<Router>...</Router>) }
if (
routesDeclaration.body.body[0].type === 'ReturnStatement' &&
routesDeclaration.body.body[0].argument?.type === 'JSXElement'
) {
const routesReturnStatement =
routesDeclaration.body.body[0].argument

if (!isAllowedElement(name)) {
context.report({
node,
messageId: 'unexpected',
data: { name },
})
checkNodes(routesReturnStatement, context)
}
}
}
}
},
}
},
})

function isRoutesRenderBlock(node?: TSESTree.VariableDeclarator) {
return (
node?.type === 'VariableDeclarator' &&
node?.id.type === 'Identifier' &&
node?.id.name === 'Routes'
)
}
Loading