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: implement require-explicit-slots #2325

Merged
merged 7 commits into from
Jan 16, 2024

Conversation

mussinbenarbia
Copy link
Contributor

This is an attempt at solving #2294

I would like to hear your opinion on the handling of slots declared twice. As you can see from the implementation, I'm reporting this as well but I'm not sure if such a thing should be responsibility of this rule.

What do you think?

Also, I did not implement a fixer for now as we do not know how the slot function is typed. I think this is good enough for now but any opinions?

Thank you.

@mussinbenarbia
Copy link
Contributor Author

mussinbenarbia commented Nov 25, 2023

Looks like the tests are failing 🤔 They all pass locally, let me see if there is some version mismatch somewhere.

EDIT: Seems to be due to node version. It fails on 14 but succeeds on 16. I'm thinking:

  • Vue 3 minimum required version is Node 16
  • This rule should only be applicable in Vue 3, due to defineSlots only working in script setup.

Maybe this is ok? I'm not sure what the protocol is for these cases so I'll wait for the reviewers' input 🙇

@FloEdelmann
Copy link
Member

We plan to drop support for Node v14 and v16 in eslint-plugin-vue v10 (see #2166), so new rules intended to be merged before that major version should be compatible with Node v14 as well. But we can also mark this PR as a breaking change and only release it with or after v10.

@mussinbenarbia
Copy link
Contributor Author

@FloEdelmann I think this makes sense to introduce in v10 then since consumers of it wouldn't be on node 14 anyway due to the reasons mentioned above 🙏

lib/rules/require-explicit-slots.js Outdated Show resolved Hide resolved
@mussinbenarbia
Copy link
Contributor Author

@ota-meshi @FloEdelmann I made some changes to the implementation based on the comments. Could you please take another look when you get the chance? 🙏

lib/rules/require-explicit-slots.js Outdated Show resolved Hide resolved
docs/rules/require-explicit-slots.md Outdated Show resolved Hide resolved
lib/rules/require-explicit-slots.js Outdated Show resolved Hide resolved
lib/rules/require-explicit-slots.js Outdated Show resolved Hide resolved
lib/rules/require-explicit-slots.js Outdated Show resolved Hide resolved
lib/rules/require-explicit-slots.js Outdated Show resolved Hide resolved
lib/rules/require-explicit-slots.js Outdated Show resolved Hide resolved
@mussinbenarbia
Copy link
Contributor Author

@ota-meshi I addressed the comments. I'm not sure if I understood the comment about the docs correctly. I'll make the change if it's still wrong

Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

docs/rules/require-explicit-slots.md Outdated Show resolved Hide resolved
docs/rules/require-explicit-slots.md Show resolved Hide resolved
Copy link
Member

@FloEdelmann FloEdelmann left a comment

Choose a reason for hiding this comment

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

Thanks for implementing the feedback! This looks good to me now 🙂

@ota-meshi ota-meshi merged commit 634f38d into vuejs:master Jan 16, 2024
16 checks passed
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.

Rule suggestion: vue/require-explicit-slots to require all slots to be documented
3 participants