search

vuejs / eslint-plugin-vue

Official ESLint plugin for Vue.js
https://eslint.vuejs.org/
MIT License
4.45k stars 666 forks source link

vue/prop-name-casing should allow "onUpdate:fooBar" #1911

Open Fuzzyma opened 2 years ago

Fuzzyma commented 2 years ago

What rule do you want to change?

vue/prop-name-casing vue/attribute-hyphenation

Does this change cause the rule to produce more or fewer warnings?

Fewer

How will the change be implemented? (New option, new default behavior, etc.)?

New default behavior

Please provide some example code that this change will affect:

<script lang="ts" setup>
const props = defineProps({
  data: {
    type: Array as PropType<Data>,
    required: true,
  },
  'onUpdate:data': {
    type: Function as PropType<(data: Data) => any | Promise<any>>,
  },
})
</script>

What does the rule currently do for this code?

This shows a warning for 'onUpdate:data' because it is not considered camelCase. However, this is the normal description of an update handler defined via v-model

What will the rule do after it's changed?

It shouldn't warn. This is a valid prop and used if you want to have more control about the v-model cycle (e.g. awaiting a promise as in this example).

This could easily be fixed by treating : as an uppercase letter.

Additional context

vue/attribute-hyphenation is related and should also be changed. It is impossible to pass a prop like :on-update:model-value because i will complain. However, its a valid prop! I am not sure if vue supports on-update as replacement for onUpdate. If not, it should be always allowed to write onUpdate

FloEdelmann commented 2 years ago

I guess you are referring to https://vuejs.org/guide/components/events.html#usage-with-v-model?

onUpdate:data should be an event, not a prop.

Fuzzyma commented 2 years ago

@FloEdelmann v-on is just syntactic sugar on top of v-bind. For more advanced usage, it is very much required that I can specify the events as props. Vue usually even tells you, that you either have to declare emits or a prop with the event name prefixed with on. So this is not hidden knowledge and very useful under some circumstances. Eslint should allow it. If you consider it bad style, than we could ofc create a new rule for it. But the current implementation of "vue/attribute-hyphenation" only has problems with FIXING. It doesn't report this kind of props as errors. Only "prop-name-casing" does and as said above, vue allows to define events like that in your props so its wrong to report it as error

FloEdelmann commented 2 years ago

For more advanced usage, it is very much required that I can specify the events as props. […] very useful under some circumstances

Why? Can you share an example?

vue allows to define events like that in your props so its wrong to report it as error

Vue also allows mixing camelCase and kebab-case for different props, but that still doesn't mean it's recommended or readable.

So this is not hidden knowledge

At least I didn't know about this.

Vue usually even tells you, that you either have to declare emits or a prop with the event name prefixed with on.

But I think in the documentation, only the "event" way is used and thus recommended, right? In https://vuejs.org/guide/components/props.html#mutating-object-array-props, the documentation states:

In most cases, the child should emit an event to let the parent perform the mutation.

@ota-meshi what's your opinion?

Fuzzyma commented 2 years ago

Why? Can you share an example?

The use case I use the most are those:

In both cases, the component could still be used with v-model without breaking anything

Vue also allows mixing camelCase and kebab-case for different props, but that still doesn't mean it's recommended or readable.

This is true, however, as I said: your rules already allow passing such a prop to a component. Only the fixer is broken. Defining such a prop yields an error but the error message is wrong. The prop is allowed. Maybe not recommended but definitely allowed. And it also follows the correct naming convention (pascal or kebap). So this rule should pass and not return an error.

At least I didn't know about this.

Its not something that you would use very often.

Vue usually even tells you, that you either have to declare emits or a prop with the event name prefixed with on.

That might be the case but again doesn't justify that the rules don't work correctly. The docs also got a few overhauls already. I think they mentioned it at some point. It's actually an essential part of how vue works under the hood

All in all, I don't see a reason not to allow it since only advanced users will ever do the props technically fulfill the requirements of the rules.

ota-meshi commented 2 years ago

If the official documentation describes how to do that, could you share it?

If it's not in the documentation, I don't think you should do it. Because I don't think it's advanced users who do things that aren't documented. If the document is simply missing, open an issue to add the document.

Fuzzyma commented 2 years ago

@ota-meshi the rules remain broken. How is this in any way related to the docs?

I cannot enable these rules without fixing being broken and getting false positives with misleading error messages. That alone is a reason good enough to fix these rules / accept the pr

ota-meshi commented 2 years ago

If it's not documented, I think it may be an unguaranteed behavior in the future. The vue/prop-name-casing rule does not accept unguaranteed behavior.

ota-meshi commented 2 years ago

However, you can do it even if it is not documented. You can also turn off the annoying eslint rule. You can also create an eslint plugin to do your favorite checks.

Fuzzyma commented 2 years ago

So using onClickas prop is fine and passing :onClick is also fine. Using @click is fine as well.

But using onUpdate:modelValue is not fine. Passing :onUpdate:modelValue is fine but breaks fixing and using @update:modelValue is perfectly valid?

This is not consistent.

If you want to restrict users from using onSomething as prop (which is your main argument), make it an extra rule (name could be "prefer-emit"). But don't leave other rules inconsistent just because of that

ota-meshi commented 2 years ago

If there is no documentation to explain it, the rule will not change it to support it. Thank you for your understanding.

Fuzzyma commented 2 years ago

@ota-meshi the rule DOES support ist!

I gave you good arguments, point out inconsistencies, discover misleading error messages and the only thing you repeat is "I can't find it in the docs". Using a prop named "onClick" is supported by your rule. So you alteady support event handlers as props. You also support passing "onUpdate:bla" without giving me any error message. It will just not make it kepap case automatically.

I get the feeling that you simply don't WANT

FloEdelmann commented 2 years ago

Using a prop named "onClick" is supported by your rule. So you alteady support event handlers as props.

That's just your interpretation. The rule doesn't care about whether a prop is used as an event handler or not. It just cares about casing. And it considers fooBar:baz as not camelCase, so it complains. onClick is definitely camelCase, so it doesn't complain.

I agree with your comment https://github.com/vuejs/eslint-plugin-vue/issues/1911#issuecomment-1154528373 though: The rules are inconsistent. @ota-meshi maybe we should indeed allow/forbid : characters and adjust the casing rules to support : in prop names in both rules.

ota-meshi commented 2 years ago

If naming with a colon is fairly common and there is a good reason, we should consider adding an option. However, if we expect it to be used as an event, other rules will need to be changed as well. For example, the vue/no-unused-properties rule should mark onClick prop as used if it was used in $emit('click').

https://eslint.vuejs.org/rules/no-unused-properties.html

FloEdelmann commented 2 years ago

For example, the vue/no-unused-properties rule should mark onClick prop as used if it was used in $emit('click').

No, because the function prop would really be unused in that case.

There would not be a real Vue event triggered by emit, but it would be calling a function that is passed as a prop. Using a colon in the prop name is just a convention for "event handler props" if I understood @Fuzzyma correctly.

Fuzzyma commented 2 years ago

technically you can invoke a function passed as prop via emit (since its all the same stuff). So @ota-meshi has a point here. If you think about render functions, you always pass down your event handlers as onSomething and still expect them to work with emit. However, I didn't have any problems with "vue/no-unused-properties" yet. If it's in "vue3-recommended", shouldn't it have complained about my code?

The only reason colon exists in prop or event names at all is because vue decided to introduce the update:fooBar methodology. It's the only place where I would ever use colons in props. e.g. onClick is an event handler prop and doesn't have any colon in it. I would even consider banning any other props with colons that don't follow the update:fooBar-semantic because it is bound to special behavior in vue. However, that would need another round of thinking because technically nobody stops you from using any colons in your prop names at all

ota-meshi commented 2 years ago

No, because the function prop would really be unused in that case.

Hmm. You are right. We may not need to change that rule. If it's just a naming issue, we may be able to add it as an option. (However, the default behavior should remain the same as it is now.)

If you really need that API for Vue core, I think you need to go ahead with the discussion on the next link.

https://github.com/vuejs/rfcs/discussions/397

Fuzzyma commented 2 years ago

If you really need that API for Vue core, I think you need to go ahead with the discussion on the next link.

The discussion seems to be about if $attrs should contain listeners but as props they are always there afaik. I will read into it some more, however. Sounds interesting.

I think adapting the rules above to streamline their behavior would clear the paths for potential other rule ideas that came up in this discussion

Fuzzyma commented 2 years ago

@ota-meshi could we probably reopen this issue? Especially after reading through your linked discussion, it seems like defining props the way I showed here seems to be even more common than I thought.

ota-meshi commented 2 years ago

For now, I think it makes the most sense to add an option instead of allowing the colon by default. However, if onUpdate:* prop is the recommended usage, it should be allowed by default, but I still can't get that confirmation.

Fuzzyma commented 2 years ago

@ota-meshi I am just thinking, that event-handler-props generally seem to be a common thing (e.g. to be able to check if a listener was passed). The only difference here is the colon in the prop name.

However, onUdpate:modelValue is the event-handler-prop that is used by v-model automatically. That's why I think, it should be allowed. Another reason why I think this should work is, that it is already allowed to pass those props to the component (because it is correctly parsed by the parser). And if that is allowed, defining those should be allowed, as well.

However, I am open to implementing an option that enables/disables props with columns. Would that be something that would make the PR megeable?

partap commented 2 years ago

My thoughts... this seems to be a "code style" rule, rather than a "code validity" rule. i.e. you can use any prop name you want and it will work... this rule just enforces a particular naming style.

The way I see it, when Vue 3 introduced update:modelValue (or update:model-value) as a standard convention, the intent is that the colon is used as a separator, and camel/kebob style should be applied to the names that are separated by the colon.

IMO, the issue of whether the name is being used as a data property or event handler is irrelevant. The linter rule is there to enforce a certain property naming style, not how it's being used. And with Vue 3, the naming convention seems to support using a colon as a separator, whether using camel or kebob case style.

ota-meshi commented 2 years ago

The way I see it, when Vue 3 introduced update:modelValue (or update:model-value) as a standard convention, the intent is that the colon is used as a separator, and camel/kebob style should be applied to the names that are separated by the colon.

I still disagree with that. The colon style was introduced only in the event name. I don't think introduced in prop name. However, you can access the event by declaring the same (similar) prop name as the event name, so use a colon for the prop. I don't think users want to use a colon for prop name for any other purpose.

If we only consider vue/prop-name-casing rule, I think we need to add one of the following options:

https://eslint.vuejs.org/rules/custom-event-name-casing.html#options

The first option can be the default, depending on the conclusion, but the second option is disabled by default. If it's a naming style issue, I think the second option makes sense.

Fuzzyma commented 2 years ago

@ota-meshi but if it is not allowed as prop, the error message should be "colon is not allowed in prop" and not "prop should be camelCase". Beside that, in a render function you would write:

const foo = ref(0)
return () => h(Component, {
  modelValue: foo.value,
  'onUpdate:modelValue': (val) => foo.value = val
})

And that is already reality. The event handler is passed as prop and has a colon

Acterion commented 2 years ago

I agree with @Fuzzyma points. The changes in the proposed PR are aimed at fixing the fixer, not checker. Both cases onUpdate:modelValue and :on-update:model-value are currently allowed by eslint if I configure it correctly (specify kebab-case or camelCase props lint rule). The problem is that fixer will not do this conversion :onUpdate:modelValue <-> :on-update:model-value automatically and will just complain instead. IMO this should be automatically fixable. The PR is valuable and currently the absence of this feature in the fixer is a friction point for me when trying to enforce kebab-case props casing in my project. As far as I can see, it doesn't break anything and only improves fixer so it's in line with what is already allowed.