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

Create direct-slot-children rule #39

Merged
merged 3 commits into from
Mar 22, 2023
Merged

Create direct-slot-children rule #39

merged 3 commits into from
Mar 22, 2023

Conversation

colebemis
Copy link
Contributor

Related to primer/react#1690

Some Primer React components use a slots pattern under the hood to render subcomponents in specific places. For example, the PageLayout component renders PageLayout.Header in the header area, and PageLayout.Footer in the footer area. These subcomponents must be direct children of the parent component, and cannot be nested inside other components.

Rule details

This rule enforces that slot components are direct children of their parent component.

👎 Examples of incorrect code for this rule:

/* eslint primer-react/direct-slot-children: "error" */
import {PageLayout} from '@primer/react'

const MyHeader = () => <PageLayout.Header>Header</PageLayout.Header>

const App = () => (
  <PageLayout>
    <MyHeader />
  </PageLayout>
)

👍 Examples of correct code for this rule:

/* eslint primer-react/direct-slot-children: "error" */
import {PageLayout} from '@primer/react'

const MyHeader = () => <div>Header</div>

const App = () => (
  <PageLayout>
    <PageLayout.Header>
      <MyHeader />
    </PageLayout.Header>
  </PageLayout>
)

@changeset-bot
Copy link

changeset-bot bot commented Mar 21, 2023

🦋 Changeset detected

Latest commit: 0bda0ed

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
eslint-plugin-primer-react Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@siddharthkp siddharthkp left a comment

Choose a reason for hiding this comment

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

Maybe I'm missing the case here, but I'm not sure if we need to maintain a stack?

Comment on lines 29 to 57
create(context) {
const stack = []
return {
JSXOpeningElement(jsxNode) {
const name = getJSXOpeningElementName(jsxNode)

// If component is a Primer component and a slot child,
// check if it's a direct child of the slot parent
if (isPrimerComponent(jsxNode.name, context.getScope(jsxNode)) && slotChildToParentMap[name]) {
const parentName = slotChildToParentMap[name]
const parent = last(stack)
if (parent !== parentName) {
context.report({
node: jsxNode,
messageId: 'directSlotChildren',
data: {childName: name, parentName}
})
}
}

// Push the current element onto the stack
stack.push(name)
},
JSXClosingElement() {
// Pop the current element off the stack
stack.pop()
}
}
}
Copy link
Member

@siddharthkp siddharthkp Mar 22, 2023

Choose a reason for hiding this comment

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

I think this can be simpler by the checking parent element?

I tried the suggestion locally and it seems to work, I can push it if you'd like

Suggested change
create(context) {
const stack = []
return {
JSXOpeningElement(jsxNode) {
const name = getJSXOpeningElementName(jsxNode)
// If component is a Primer component and a slot child,
// check if it's a direct child of the slot parent
if (isPrimerComponent(jsxNode.name, context.getScope(jsxNode)) && slotChildToParentMap[name]) {
const parentName = slotChildToParentMap[name]
const parent = last(stack)
if (parent !== parentName) {
context.report({
node: jsxNode,
messageId: 'directSlotChildren',
data: {childName: name, parentName}
})
}
}
// Push the current element onto the stack
stack.push(name)
},
JSXClosingElement() {
// Pop the current element off the stack
stack.pop()
}
}
}
create(context) {
return {
JSXOpeningElement(jsxNode) {
const name = getJSXOpeningElementName(jsxNode)
// If component is a Primer component and a slot child,
// check if it's a direct child of the slot parent
if (isPrimerComponent(jsxNode.name, context.getScope(jsxNode)) && slotChildToParentMap[name]) {
const JSXElement = jsxNode.parent
const parent = JSXElement.parent
const expectedParentName = slotChildToParentMap[name]
if (parent.type !== 'JSXElement' || getJSXOpeningElementName(parent.openingElement) !== expectedParentName) {
context.report({
node: jsxNode,
messageId: 'directSlotChildren',
data: {childName: name, parentName: expectedParentName}
})
}
}
}
}
}

@colebemis colebemis merged commit 015237e into main Mar 22, 2023
This was referenced Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants