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

feat(no-duplicate-attr-inheritance): ignore multi root #2598

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

waynzh
Copy link
Contributor

@waynzh waynzh commented Nov 5, 2024

resolve #2596.

  • since this will result in fewer errors, should we still consider adding an option to control this?
  • multi root could also be a group of v-if / v-else / v-else nodes. Should we consider treating it as a "single" root?

@FloEdelmann
Copy link
Member

  • since this will result in fewer errors, should we still consider adding an option to control this?

I think it would make sense to add a new checkMultiRootNodes option (false by default).

  • multi root could also be a group of v-if / v-else / v-else nodes. Should we consider treating it as a "single" root?

How does Vue itself behave here?

@waynzh
Copy link
Contributor Author

waynzh commented Nov 10, 2024

add a new checkMultiRootNodes option (false by default).

When checkMultiRootNodes is set to true, this rule will ignore the multi root nodes error. Perhaps we could consider renaming it to something like ignoreMultiRootNodes?

How does Vue itself behave here?

Vue treats a group of v-if / v-else-if/v-else as a single node. I've added an isConditionalGroup function to do that.

@FloEdelmann
Copy link
Member

When checkMultiRootNodes is set to true, this rule will ignore the multi root nodes error. Perhaps we could consider renaming it to something like ignoreMultiRootNodes?

Sorry, my suggestion was not clear enough. I'd say the rule should stop reporting errors for components with multiple root nodes by default. But the old behavior should still be available (for users who want to explicitly define the behavior for multi-root components) via an option. Is checkMultiRootNodes a good name for that? Do you have a better suggestion?

@waynzh
Copy link
Contributor Author

waynzh commented Nov 12, 2024

the rule should stop reporting errors for components with multiple root nodes by default

Then the name and setting the default to false make sense to me. I've updated the test cases and docs.

Comment on lines +11 to +34
let hasIf = false
let hasElse = false

for (const element of elements) {
if (utils.hasDirective(element, 'if')) {
if (hasIf || hasElse) {
return false
}
hasIf = true
} else if (utils.hasDirective(element, 'else-if')) {
if (!hasIf || hasElse) {
return false
}
} else if (utils.hasDirective(element, 'else')) {
if (!hasIf || hasElse) {
return false
}
hasElse = true
} else {
return false
}
}

return hasIf && (elements.length === 1 || hasElse)
Copy link
Member

Choose a reason for hiding this comment

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

I think this is much more readable (and we don't need to care about arrays with less than 2 entries anyway):

Suggested change
let hasIf = false
let hasElse = false
for (const element of elements) {
if (utils.hasDirective(element, 'if')) {
if (hasIf || hasElse) {
return false
}
hasIf = true
} else if (utils.hasDirective(element, 'else-if')) {
if (!hasIf || hasElse) {
return false
}
} else if (utils.hasDirective(element, 'else')) {
if (!hasIf || hasElse) {
return false
}
hasElse = true
} else {
return false
}
}
return hasIf && (elements.length === 1 || hasElse)
if (elements.length < 2) {
return false
}
const firstElement = element[0]
const lastElement = elements[elements.length - 1]
const inBetweenElements = elements.slice(1, -1)
return (
utils.hasDirective(firstElement, 'if') &&
(utils.hasDirective(lastElement, 'else-if') ||
utils.hasDirective(lastElement, 'else')) &&
inBetweenElements.every((element) => utils.hasDirective(element, 'else-if'))
)


</eslint-code-block>

### `"checkMultiRootNodes": true`
Copy link
Member

Choose a reason for hiding this comment

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

This heading should be below the options.

}
```

- `"checkMultiRootNodes"` ... If `true`, check and warn when there are multiple root nodes. (Default: `false`)
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add an explanation why the default is false (and that is fine), and why someone might still want to enable the option?

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.

no-duplicate-attr-inheritance: should ignore components with multiple root nodes?
2 participants