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

vue/no-setup-props-destructure is too restrictive after latest update #2259

Closed
l3d00m opened this issue Jul 30, 2023 · 34 comments · Fixed by #2268
Closed

vue/no-setup-props-destructure is too restrictive after latest update #2259

l3d00m opened this issue Jul 30, 2023 · 34 comments · Fixed by #2268

Comments

@l3d00m
Copy link

l3d00m commented Jul 30, 2023

What rule do you want to change?

vue/no-setup-props-destructure

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.)?

Revert #2244 and extract those rule changes into a new rule.

The changes now cause rule violations for example code directly from the vue documentation (see #2244 (comment)).

It now enforces more or less subjective defaults. I mostly agree with the defaults, but the rule is active in vue3-essential, which shouldn't happen for subjective defaults.

In addition to that, the name and documentation no longer matches with the behaviour.

Please provide some example code that this change will affect:

<script setup>
  const props = defineProps({ count: Number })
  const count = ref(props.count)
                         ^^^^^^ Failing
</script>

What does the rule currently do for this code?

The code is violating the rule, even though this is not a destructure, but a non reactive access.

What will the rule do after it's changed?

It shouldn't violate. This is a valid use case for setting an initial value, even though it might not be required to do so often. Still, it shouldn't be necessary to disable the rule for every line where this occurs.

Please consider making this a new rule, which could be placed in recommended instead. This has nothing to do with destructuring anymore, which is indeed critical and should fail.

@l3d00m l3d00m changed the title vue/no-setup-props-destructure vue/no-setup-props-destructure is too restrictive after latest update Jul 30, 2023
@websmurf
Copy link

We are also running into issues with the last updated version, for example:

<script lang="ts">
export default defineComponent({
  props: {
    canSave: {
      type: Boolean,
      default: true,
    },
    showSave: {
      type: Boolean,
      default: true,
    },
  },
  setup (props) {
    const disabled = ref<boolean>(props.canSave === false)
                                  ^^ Failing

or

<script lang="ts">
export default defineComponent({
  props: {
    id: {
      type: String,
      required: true,
    },
    modelValue: {
      type: Boolean,
      required: true,
    },
    noLabel: {
      type: String,
      required: true,
    },
    yesLabel: {
      type: String,
      required: true,
    },
  },
  emits: ['update:modelValue'],
  setup (props, { emit }) {
    ...
    const options: RadioInputItem[] = ref([
                                      ^^ Failing
      { value: false, label: props.noLabel },
      { value: true, label: props.yesLabel },
    ])

@daniluk4000
Copy link

daniluk4000 commented Jul 31, 2023

Same. We use prop value as default value for some refs. It would be nice if we could exclude this case via additional option.

@l3d00m
Copy link
Author

l3d00m commented Jul 31, 2023

It would be nice if we could exclude this case via additional option.

Please, do not make this an additional option. As stated in the issue, the linting is completely unrelated to what vue/no-setup-props-destructure implies so it should be a new rule, rather than an option.

It might be a good idea to revert the #2244 as a quick patch release (and extract it into a proper new rule with the next major/minor release), since this issue seems to affect a lot of people.

@ota-meshi
Copy link
Member

ota-meshi commented Jul 31, 2023

The vue/no-setup-props-destructure rule warns of statements that lose reactivity of props values.
https://eslint.vuejs.org/rules/no-setup-props-destructure.html#rule-details
So I think the current rule is working correctly.
You can turn off the rule if you intentionally want to lose props reactivity.
If people don't find this rule useful, I think we can consider removing it from the shareable configuration provided by this plugin.
Also, if the sharable configuration provided by this plugin is not useful for people, we may consider deprecating the sharable configuration provided by the plugin in the next version.

@jacekkarczmarczyk
Copy link

I'm not personally against the new behaviour, but the name of the rule doesn't match this behaviour

@daniluk4000
Copy link

The vue/no-setup-props-destructure rule warns of statements that lose reactivity of props values.

I'm okay with it's warning on top-level or in arguments - that's very dangerous and could lead to errors.

But in above cases we only trying to set default value for refs. We don't really need reactivity here. My suggestion above was to cover this with an additional options, others thought about complete new rule.

Complete disabling this rule for this new added case could also lead to many unwanted developer errors. We need the rule to cover specific places and not to cover ref assigment (at least with possibility to disable other rule / disable this behavior via some option).

@l3d00m
Copy link
Author

l3d00m commented Jul 31, 2023

The vue/no-setup-props-destructure rule warns of statements that lose reactivity of props values.

Sorry, but this is not true. Your links explicitly states that the rule "reports the destructuring of props passed to setup causing the value to lose reactivity".

And my complaint was specifically about the newly added access linting.

You can turn off the rule if you intentionally want to lose props reactivity.

No, because this rule is incredibly useful! Erroring on destructuring is important for me. Just the new changes are not.

Also, if the sharable configuration provided by this plugin is not useful for people, we may consider deprecating the sharable configuration provided by the plugin in the next version.

I don't understand where you got that from. The only fact so many people are complaining here is because probably everyone uses the shareable config because they are incredibly helpful. I really do appreciate them.

Please understand that I'm not coming from a hostile position. I ask you to reconsider splitting the rules.

The original rule is important and it makes sense to have it as an error rule in essential. The new changes though are subjective and should probably just emit a warning. And they most certainly shouldn't be placed in essential as an error. As a user there's currently no way but to disable the rules for both use cases.

@kleinfreund
Copy link

kleinfreund commented Jul 31, 2023

While I understand the frustration this brings with it, the rule as it used to work is incomplete in my opinion. Sure, the name is misleading now, but both destructuring of props and initialization of new refs using props via property access break in the same way (i.e. with regards to reactivity being lost):

const newRef1 = ref(props.test)

const { test } = props
const newRef2 = ref(test)

Neither of the created refs are reactive based on props.test.

There is a simple solution for making a reactive ref out of a prop: toRef

const newRef3 = toRef(() => props.test)

If (and multiple people point this out here) the risk of unintentional loss of reactivity through destructuring of props is real and should lead to an error, the same should be true for other ways the same issue can occur. The changed rule addresses this.

@daniluk4000
Copy link

daniluk4000 commented Jul 31, 2023

There is a simple solution though: toRef

toRef will create readonly ref from my prop, which is not the case. I'll share the example.

const localValue = ref(props.modelValue);
const { modelValue } = props;

const handleValueUpdate = (value: string) => {
    emit('update:modelValue', value);
}

 // Watch and set actual value here
watch(() => props.modelValue, () => localValue.value = props.modelValue);

// Oops! Value remains old and will never change since we used destructure
watch(() => modelValue, () => localValue.value = modelValue);

As you can see in this example, destructuring of props would break my watch as well as anything else. Smart use of top-level assigment would not lead to this thought. What will be your solution for this case?

Also, let's take a look on OAPI realization.

<script lang="ts">
import { defineComponent } from 'vue';

export default defineComponent({
     props: {
          modelValue: {
              type: String,
              required: true,
          },
     },
     data() {
          return {
               localValue: this.modelValue,
          },
     },
     watch: {
         modelValue() {
              this.localValue = this.modelValue;
         },
     },
     methods: {
          handleValueUpdate(value: string) {
               this.$emit('update:modelValue', value);
          },
     },
});
</script>

This is one of classic cases when we need to set default value for our changeable data.

@kleinfreund
Copy link

kleinfreund commented Jul 31, 2023

toRef will create readonly ref from my prop

It's intentional that props aren't allowed to be mutated, hence toRef(() => props.test) won't allow you to modify props.test.


Looking at this more, it seems to at least be hard to replicate some very common uses cases while leaving the rule active:

const newRef = ref(props.test)

This results in an error as many people pointed out.

But how can one declare that they're fine with losing the reactivity in this example?

const newRef = toRef(props.test)

This works, but you can't mutate newRef. What if you want a local piece of reactive state that you can mutate that's initialized with a prop value?

const propValue = toValue(props.test)
const newRef = ref(propValue)

Unfortunately, the call to toValue(props.test) produces the rule error.

Annoying workaround: Move the props access out of the top-level of the component script:

const newRef = ref(getDefaultValue())

function getDefaultValue () {
  return props.test
}

getCryptoAddress added a commit to getCryptoAddress/getCryptoAddress.github.io that referenced this issue Jul 31, 2023
@davidmyersdev
Copy link
Contributor

davidmyersdev commented Aug 1, 2023

I'd like to point out that vue/no-ref-object-destructure actually behaves like this rules does after these changes. In fact, I based my PR on that rule after I noticed the disparity between them. That said, I also think there is confusion here with the meaning of destructuring in this context. Take the following example:

const foo = { bar: 'baz' }
const { bar } = foo

It could just as easily be rewritten as this:

const foo = { bar: 'baz' }
const bar = foo.bar

No, the latter example is not a destructuring assignment, but it accomplishes the same thing. It destructures an object's property accesses into distinct references to the underlying objects or values, which breaks the ability to track said accesses (hence why it breaks reactivity). In fact, if you look at the tests from before I modified the rule, they explicitly test that the latter example produces an error. Preventing the accidental breakage of reactivity is likely why you're using this rule, otherwise disabling it is a simple option here.

That said, yes, it makes sense to support cases where you're making a conscious decision to not keep the reactivity of props (or at least handle it differently), so let's dig into what you can do right now without a change to this rule.

But how can one declare that they're fine with losing the reactivity in this example?

const newRef = toRef(props.test)

While some have pointed out the toRef example, the main complaint is that using it will cause you to lose the ability to update the underly props value. I'd like to recommend you don't do that, as the Vue docs clearly state that props flow one way, but I know that's not always practical in a pre-existing codebase. Instead, I'd recommend you look at one of the overloads of toRef that allows you to maintain that 2-way data flow:

// Specify the prop as the second argument
const newRef = toRef(props, 'test')

My goal in making this change was not to ruin anyone's day. It was actually quite the opposite. As someone who loves Vue, I like to do my part to teach others about how it works, and a big part of that is the reactivity system. When beginners come to Vue and they don't fully understand how it works or they come from frameworks like React (where things are automatically reactive, albeit at the cost of performance), it's not difficult for them to break reactivity. When that happens, especially in a professional setting, it can be really discouraging for them. The goal here is to help catch errors like these and teach others in the process so that they can feel empowered to work in a Vue codebase with less risk.

@ota-meshi
Copy link
Member

I'm not personally against the new behaviour, but the name of the rule doesn't match this behaviour.

Yeah. I agree that the rule name is confusing. When the rule was implemented, I thought destructuring was the cause of losing reactivity, but it wasn't the only one.
We may need to change to the new rule name. Can you suggest a better rule name?

@ddenev
Copy link

ddenev commented Aug 1, 2023

I'd like to point out that vue/no-ref-object-destructure actually behaves like this rules does after these changes. In fact, I based my PR on that rule after I noticed the disparity between them. That said, I also think there is confusion here with the meaning of destructuring in this context. Take the following example:

const foo = { bar: 'baz' }
const { bar } = foo

It could just as easily be rewritten as this:

const foo = { bar: 'baz' }
const bar = foo.bar

No, the latter example is not a destructuring assignment, but it accomplishes the same thing. It destructures an object's property accesses into distinct references to the underlying objects or values, which breaks the ability to track said accesses (hence why it breaks reactivity). In fact, if you look at the tests from before I modified the rule, they explicitly test that the latter example produces an error. Preventing the accidental breakage of reactivity is likely why you're using this rule, otherwise disabling it is a simple option here.

That said, yes, it makes sense to support cases where you're making a conscious decision to not keep the reactivity of props (or at least handle it differently), so let's dig into what you can do right now without a change to this rule.

But how can one declare that they're fine with losing the reactivity in this example?

const newRef = toRef(props.test)

While some have pointed out the toRef example, the main complaint is that using it will cause you to lose the ability to update the underly props value. I'd like to recommend you don't do that, as the Vue docs clearly state that props flow one way, but I know that's not always practical in a pre-existing codebase. Instead, I'd recommend you look at one of the overloads of toRef that allows you to maintain that 2-way data flow:

// Specify the prop as the second argument
const newRef = toRef(props, 'test')

My goal in making this change was not to ruin anyone's day. It was actually quite the opposite. As someone who loves Vue, I like to do my part to teach others about how it works, and a big part of that is the reactivity system. When beginners come to Vue and they don't fully understand how it works or they come from frameworks like React (where things are automatically reactive, albeit at the cost of performance), it's not difficult for them to break reactivity. When that happens, especially in a professional setting, it can be really discouraging for them. The goal here is to help catch errors like these and teach others in the process so that they can feel empowered to work in a Vue codebase with less risk.

That is a very good explanation of the case and thank you very much, @davidmyersdev, for that. Now it is clearer to me the reasoning behind this change and it makes more sense to cover the "losing reactivity" case. It's only the name of the rule that "gets in the way" but that's easily solvable.

What I was missing previously was how to rewrite my code so that I still can init my local state from props, not caring about reactivity, and not being bombarded by lint about these errors.
With your examples above this is now solved.

Therefore, one additional suggestion - what you specified above (the 2 approaches to initializing a local ref from props) is a VERY good candidate to be incorporated into the official Vue docs, to replace the current part about that topic. If you do that, IMHO the circle will be complete :)

@ddenev
Copy link

ddenev commented Aug 1, 2023

@davidmyersdev - one problem though :)

I just tested and this still fails with the new rule:

const newRef = toRef(props.test)

I guess you'll need to update the rule to ease on that case, right?

@jacekkarczmarczyk
Copy link

We may need to change to the new rule name

I think this whole discussion suggest that this rule's name should stay, just the new behaviour should be reverted, and there should be a new rule which would forbid non-reactive assignments (how about vue/no-non-reactive-assignments as a name?) like const { foo } = props or const foo = ref(props.foo).
Would it be difficult to detect all such assignments, not only related to props?

const foo = someComputed.value;
const bar = anyReactive.someProp;
// etc

Of course the assignment should be outside of some effect to be reported, not sure how hard/easy it is to implement

@ota-meshi
Copy link
Member

I have no plans to create new rule. As @kleinfreund wrote, the following two statements both lose reactivity.

const newRef1 = ref(props.test)

const { test } = props
const newRef2 = ref(test)

I don't see any reason to allow only newRef1.

@richardsimko
Copy link

But then what is the canonical way of loading a prop as a default value in a ref or reactive object if I don't care about reactivity in the prop? Simply sprinkle the code with lint ignores?

  const state = reactive<{
    foo: string;
    bar: number;
  }>({
    // This works perfectly fine for loading a default value but will error with the new rule
    foo: props.modelValue.foo,
    isUnlocked: someOtherDefault,
  });

@ota-meshi
Copy link
Member

If you intentionally want to lose reactivity, you should either turn off the rule or write a suppress comment.

@jacekkarczmarczyk
Copy link

If you intentionally want to lose reactivity, you should either turn off the rule or write a suppress comment.

I'd agree with that if the name of the rule was different, but it is what it is and now it's misleading. And changing the name of the rule would be a breaking change.

@ota-meshi
Copy link
Member

ota-meshi commented Aug 1, 2023

I already agree that the name of the rule is confusing. We have to rename the rule.
It will not be a breaking change. Copy the vue/no-setup-props-destructure rule to create a rule with a new name and deprecate the vue/no-setup-props-destructure rule. Remove deprecated rules in major version.

@FloEdelmann
Copy link
Member

See #2222 for a similar rule renaming.

@AndreiSoroka
Copy link

AndreiSoroka commented Aug 1, 2023

I already agree that the name of the rule is confusing. We have to rename the rule. It will not be a breaking change. Copy the vue/no-setup-props-destructure rule to create a rule with a new name and deprecate the vue/no-setup-props-destructure rule. Remove deprecated rules in major version.

But vue/no-setup-props-destructure is a clear and understandable rule.
No need to merge with new logic, better to create the new rule.
P.s. Smaller rules are easier to manage, because easier to turn off

"rules": {
  "vue/new-rule": "off " // Good,
  // "vue/no-setup-props-destructure": "on" // from default
}

but not

"rules": {
   "vue/new-rule": ["error", null, { "subrule_to_turn_off_only_new_logic": false}] // Bad
}

just agree that it was a critical change

@AndreiSoroka
Copy link

AndreiSoroka commented Aug 1, 2023

I think

Revert #2244 and extract those rule changes into a new rule.

Is a good way for developing

  1. Revert
  2. Take a time for better solution
  3. Release

no one needs a hasty decision now,
and expectation/inactivity is affecting everyday more and more projects

@ota-meshi
Copy link
Member

I think we need to make the following changes.

  • Rename that rule. (Minor versions have rules with both names. Mark no-setup-props-destructure as deprecated.)
  • Remove that rule from shareable configurations.

Can someone please suggest a new rule name?


I have no plans to revert the vue/no-setup-props-destructure rule. because that rule will soon be deprecated.

@kleinfreund
Copy link

Can someone please suggest a new rule name?

Perhaps vue/no-setup-props-reactivity-loss.

This covers the new behavior better.

However, I do think that this is quite the subjective rule. I reckon that far more people have code like const newRef = ref(props.test) than code like const { test } = defineProps(['test']). For this group, the rule will likely have lost its value as there isn't currently a good way to both keep the rule active and not always disable it whenever you try to initialize a new ref with a prop as its initial value.

Personally I think this change should’ve shipped as a breaking change so people had a clear point at which they could decide whether the rule has value for them.

Also, just to make this clear, I do think the presets this plugin provides are very useful and I definitely think it should continue shipping them.

@AndreiSoroka
Copy link

AndreiSoroka commented Aug 1, 2023

because that rule will soon be deprecated

Could you please explain why? Why need to expand the rule, but not create new rules?

Like this

{
  "vue/no-setup-props-destructure": "on",
  "vue/no-reactivity-loss-on-props-ref": "off"
}
// "vue/no-setup-props-destructure". It is destructuring. It is the correct error
const { test } = defineProps<{test: string}>()
// Because people may think that the test stay reactive

// Why it is should stay reactive?
const newRef1 = ref(props.test)

// And this?
const newRef3 = ref(123)
const notReactivityRef = newRef3.value // destructure?
const newRef4 = ref(notReactivityRef)

// If you want reactivity, then need to use computed, but not ref
const newRef5 = computed(()=>props.test)
// It is obvious behavior. I am not sure it's correct rule

Can someone please suggest a new rule name?

P.s. From Chat GPT variants

  • vue/no-reactivity-loss-on-props-ref
  • vue/avoid-props-ref-reativity-loss
  • vue/no-props-to-ref-directly
  • vue/prevent-ref-from-props
  • vue/props-direct-ref-disallowed
  • vue/no-direct-props-ref

IMHO: Again. Just need to revert

@richardsimko
Copy link

richardsimko commented Aug 1, 2023

If you intentionally want to lose reactivity, you should either turn off the rule or write a suppress comment.

So you mean that props as defaults for mutable state is an invalid approach? Because I think I've seen it recommended in the Vue docs, although I may be wrong.

To me disabling a default rule implies I'm doing something that goes against best practices. If assigning a prop value as a default value is against best practices, then what is the best practice? How would you solve the same case without disabling this rule?

@jacekkarczmarczyk
Copy link

@richardsimko defineModel, useVModel from vue-use etc

@cailloumajor
Copy link

cailloumajor commented Aug 1, 2023

@richardsimko defineModel, useVModel from vue-use etc

@jacekkarczmarczyk I don't think this answers @richardsimko need (that is, correct me if I'm wrong, to have a one-time one-way binding from parent to component). Your answer, if I am not mistaken, is about two-way binding.

P.S.: I am in the same situation, that is, to have a one-time one-way binding (providing a default value in other words), and I would be very happy to see a separate rule for this case, preventing the disabling of the existing, rightly recommended, rule.

@lianee
Copy link

lianee commented Aug 1, 2023

I'm surprised nobody actually mentionned the fact that this rule also triggers when props "value" are passed to a function.
What is the point of erroring on a simple console.log(props.foo), and what is the use to pass reactive values to axios calls?
Also, I think that basic level of checking rules should not error for any example in Vue site. Next levels could be more opinionated, but the basic ones should comply with official Vue recipes.
For now, I totally deactivated this rule, I won't refactor perfectly legal code for this subjective rule.

@ota-meshi
Copy link
Member

You can turn off the rule if you intentionally want to lose reactivity.

@ota-meshi
Copy link
Member

Thanks to those who suggested new rule names.
I have already commented how to modify this plugin to solve this issue.
If you don't like it, you can fork this project.
I don't want to receive any more comments, so I'm locking this issue.

@l3d00m

This comment was marked as resolved.

@vuejs vuejs locked as too heated and limited conversation to collaborators Aug 1, 2023
@FloEdelmann
Copy link
Member

The rule has now been renamed to vue/no-setup-props-reactivity-loss and removed from any preset configs.
See https://github.com/vuejs/eslint-plugin-vue/releases/tag/v9.17.0.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet