Skip to content

feat: add svelte/require-each-key rule #473

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

Merged
merged 2 commits into from
May 10, 2023
Merged

feat: add svelte/require-each-key rule #473

merged 2 commits into from
May 10, 2023

Conversation

ota-meshi
Copy link
Member

close #459

@changeset-bot
Copy link

changeset-bot bot commented May 9, 2023

🦋 Changeset detected

Latest commit: b098c06

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-svelte Minor

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

@baseballyama baseballyama left a comment

Choose a reason for hiding this comment

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

I thought I would like to check if the key was a variable used in a #{each} tag.

e.g.

<script>
  let foo;
  let bar;
</script>
<!-- This is NG -->
{#each foo as name (bar)}...{/each}

@ota-meshi
Copy link
Member Author

As you said, I think it would be useful for a rule to check if a key contains an iteration variable 👍
However, I think it would be better to implement the check as a separate rule.
Because even if we don't want the key to be required, we want the rule to check if the key contains an iteration variable.

docs: {
description: "require keyed `{#each}` block",
category: "Best Practices",
recommended: false,
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 we can toggle to true in major version up?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. I personally like to enforce the key, but vue/require-v-for-key is unpopular even in eslint-plugin-vue, so I think it's better not to include it in the preset. (However, the Vue style guide recommends using keys.)
I think it will be easier to introduce eslint if the presets are not too strict.

Copy link
Member

Choose a reason for hiding this comment

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

Okay!

Copy link
Member

@baseballyama baseballyama left a comment

Choose a reason for hiding this comment

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

I wrote one comment but LGTM!

@ota-meshi ota-meshi merged commit 6b71add into main May 10, 2023
@ota-meshi ota-meshi deleted the each-key branch May 10, 2023 03:20
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.

New Rule: svelte/require-each-key
2 participants