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

fix(reactivity): ensure computed wrapped in readonly still works (close #3376) #3377

Closed
wants to merge 2 commits into from

Conversation

LinusBorg
Copy link
Member

Wrapping a computed() in readonly() breaks the computed's functionality.

This PR ensures that such a wrapped computed can still set its internal proeprties _dirty & _value, ensuring it can continue to work as expected.

close #3376

@github-actions
Copy link

github-actions bot commented Mar 6, 2021

Size report

Path Size
vue.global.prod.js 40.93 KB (+0.11% 🔺)
runtime-dom.global.prod.js 27.2 KB (+0.13% 🔺)
size-check.global.prod.js 16.51 KB (+0.26% 🔺)

@LinusBorg LinusBorg added ❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. scope: reactivity 🔍 review needed labels Mar 6, 2021
@LinusBorg
Copy link
Member Author

I added a test but would like input if there are more scenarios to cover.

Comment on lines 202 to 206
if ((target as any).__v_isRef && (target as any).effect) {
// computed should be able to set its own private properties
if (key === '_dirty' || key === '_value') {
return set(target, key, value, receiver)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we add __v_isComputed to ComputedRefImpl? like this:

class ComputedRefImpl<T> {
  private _value!: T
  private _dirty = true

  public readonly effect: ReactiveEffect<T>

  public readonly __v_isRef = true;
+  public readonly __v_isComputed = true;
  public readonly [ReactiveFlags.IS_READONLY]: boolean
}

I mean, this would be safer, consider the following scenario:

const data = ref({ effect: true })
const rData = readonly(data)

rData._value = 'xxx' // Unexpectedly 

with the __v_isComputed, we can implement the isComputed function, just like isRef.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm a bit confused by the {effect: true} - that shouldn't have an impact, as that would be in target.value.effect, wouldn't it?

But nonetheless propably a safer approach., I agree.

Copy link
Member

Choose a reason for hiding this comment

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

Oh sure, but you know what i mean 😄

@LinusBorg LinusBorg self-assigned this Mar 11, 2021
@LinusBorg LinusBorg force-pushed the linusborg/fix-readonly-computed-3376 branch from ebe7c45 to 67f89fd Compare March 20, 2021 15:09
@LinusBorg
Copy link
Member Author

@HcySunYang I did the change you proposed, can you have a look? Thanks!

@yyx990803
Copy link
Member

See cleaner fix in 41e02f0

@yyx990803 yyx990803 closed this Mar 25, 2021
@LinusBorg
Copy link
Member Author

Definitely the better solution, thanks.

@yyx990803 yyx990803 deleted the linusborg/fix-readonly-computed-3376 branch June 8, 2021 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. scope: reactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrapping a computed in readonly() causes console warnings and breaks the computed's functionality
3 participants