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

Add rule require-event-prefix #910

Open
marekdedic opened this issue Nov 8, 2024 · 10 comments
Open

Add rule require-event-prefix #910

marekdedic opened this issue Nov 8, 2024 · 10 comments
Labels
enhancement New feature or request new rule

Comments

@marekdedic
Copy link
Contributor

marekdedic commented Nov 8, 2024

Motivation

In Svelte 5, all HTML element events are props starting with "on". Component events can have any name, so it is not clear, which props are events and which are not.

Description

I'd like to add a rule that requires that all props that are events (i.e. that are functions) start with "on"

Examples

<script lang="ts">
  /* ✓ GOOD */

  interface Props {
    regularProp: string;
    onclick(): void;
  }

  let { regularProp, onclick }: Props = $props;

  /* ✗ BAD */

  interface Props {
    click(): void;
  }

  let { click }: Props = $props;
</script>

Additional comments

I think this works even without TS, as the type would be in the parsed AST, right?

@marekdedic marekdedic added enhancement New feature or request new rule labels Nov 8, 2024
@ota-meshi
Copy link
Member

Hmm. I think it would be useful to have that rule, but I'm worried about whether it's okay to judge all function props as event props 🤔.

What about implementing a rule that checks naming conventions for all props and provides examples for event names in the rule's documentation?
The rule could also disallow the on prefix for non-function props.

@marekdedic
Copy link
Contributor Author

Well, in Svelte 5, IIUC there are no special event props - it's all just props, functions or not... I think in that case, it makes sense to treat function props as events...

Disallowing the on prefix for non-function props makes sense to me 👍

I don't really understand your point about naming convention for all props?

@ota-meshi
Copy link
Member

I don't really understand your point about naming convention for all props?

For example, the following rule:

{
  "svelte/props-naming-convention": [
    "error",
    {
      "pattern": [
        "/^(?!on)/u" // Non-function props should not start with `on`.
      ],
      "overrides": [
        {
          "type": "function",
          "pattern": [
            "/^on/u" // Function props must start with `on`.
          ]
        },
      ]
    }
  ]
}
<script>
  /* ✓ GOOD */
  let { onclick = () => {}, num = 42 } = $props();
  /* ✗ BAD */
  let { onFoo = 42, bar = () => {} } = $props();
</script>
{
  "svelte/props-naming-convention": [
    "error",
    {
      "pattern": [...],
      "overrides": [
        {
          "type": "boolean",
          "pattern": [
            "/^is/u" // Boolean props must start with `is`.
          ],
        },
      ]
    }
  ]
}
<script>
  /* ✓ GOOD */
  let { isEnable = true } = $props();
  /* ✗ BAD */
  let { enable = true } = $props();
</script>

@marekdedic
Copy link
Contributor Author

Uff. If we are doing a general naming convention rule in the style of @typescript-eslint/naming-convention, it would be probably good to apply it to things other than props (e.g. you could have a separate convention for all runes?)

That is a quite large undertaking and I am not sure it's something the users want (typescript-eslint has recently stopped adding features to stylistic rules altogether due to this...). So in this instance I would be inclined to add a much simpler rule that enforces events to start with on and possibly non-event props to not start with on. The more general naming convention rule is a can that should, in my opinion, kicked down the road.

@baseballyama
Copy link
Member

Passing functions as props is not just for events but also for dependency injection (DI)-like use cases.
This rule feels too restrictive and lacks flexibility.

@marekdedic
Copy link
Contributor Author

Hmm, I'm not familiar with that use case, could you please share an example? I'm trying to come up with a design that isn't too restrictive, yet at the same time isn't a full blown naming convention... Thanks!

@baseballyama
Copy link
Member

baseballyama commented Jan 2, 2025

This is example of it.
Then we can mock API calling for component test.

<script>
  const { fetchData } = $props();
  let data = $state({});
  const onclick = async () => {
    data = await fetchData();
  }
</script>

<button {onclick}>Click Me</button>

@marekdedic
Copy link
Contributor Author

Thanks, I assume you mean onclick is an event and fetchData is not, right?

In your example, fetchData is async - to me, async functions don't feel like events (same for generator functions), but I suspect that isn't enough of a limitation.

I still think the proposed idea has value for some users, but you are right that it is subjective and there will be people who don't want this. At the same time, I don't really see a good way to distinguish what feels like an event and what doesn't. Do you have any things that make fetchData not an event in your mind?

@baseballyama
Copy link
Member

I’m not sure😅. I often write frontend code at work, and it’s common to pass async functions as event handlers.

@marekdedic
Copy link
Contributor Author

And do you then await them in the component? That seems very not-event-like to me...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request new rule
Projects
None yet
Development

No branches or pull requests

3 participants