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

readonly(["a", "b", "c"]).includes() tracks dependencies even though array is nonreactive #2493

Closed
lixiaofa opened this issue Oct 27, 2020 · 15 comments · Fixed by #2506
Closed
Labels
has PR A pull request has already been submitted to solve the issue 🐞 bug Something isn't working scope: reactivity

Comments

@lixiaofa
Copy link

Version

3.0.2

Reproduction link

https://codesandbox.io/s/vigorous-field-e0xxe?file=/src/App.vue

Steps to reproduce

readonly(["a", "b", "c"]) Collected by dependency

What is expected?

["a", "b", "c"] Should not be collected by dependency

What is actually happening?

["a", "b", "c"] Collected by dependency

@LinusBorg
Copy link
Member

LinusBorg commented Oct 27, 2020

You misunderstand what readonly does. This is expected behaviour. readonly still allows for values to be collected as dependencies - it only keep you from mutating the source objects through the procxy returned by readonly.

Maybe you are looking for markRaw() or one of the shallow*() APIs.

Edit: my analysis here was wrong, see below (#2493 (comment))

@lixiaofa
Copy link
Author

I'd like to ask why this design is so designed that indexes can't be collected by dependency, but by using lastIndexOf, indexof and includes. Shouldn't readonly behave consistently

@LinusBorg
Copy link
Member

Please be more specific and provide an example.

@lixiaofa
Copy link
Author

setup() {
const readonlyState = readonly(["a", "b", "c"]);
effect(() => {
debugger;
readonlyState.includes("a");
});
},
so
readonly(["a", "b", "c"]) Collected by dependency

if
setup() {
const readonlyState = readonly(["a", "b", "c"]);
effect(() => {
debugger;
readonlyState[0]
});
},
readonly(["a", "b", "c"]) Should not be collected by dependency

Shouldn't readonly behave consistently

@lixiaofa

This comment has been minimized.

@LinusBorg

This comment has been minimized.

@lixiaofa

This comment has been minimized.

@lixiaofa

This comment has been minimized.

@LinusBorg

This comment has been minimized.

@LinusBorg
Copy link
Member

LinusBorg commented Oct 28, 2020

Ah, now I get what you mean. I failed at reproducing it properly first with the snippet your provided.

Let's look into it, we might have an issue here.

@LinusBorg LinusBorg reopened this Oct 28, 2020
@LinusBorg LinusBorg self-assigned this Oct 28, 2020
@lixiaofa
Copy link
Author

You may have misunderstood the meaning

In the case of readonly(),

Why? readonlyState.includes ("a") to collect dependencies, readonlystate [0] will not collect dependencies
Not consistent behavior

@lixiaofa
Copy link
Author

OK, thank you

@LinusBorg
Copy link
Member

So thanks for being adamant here, I was wrong.

readonly on a plain value should of course not track. I usually use it on reactive values only and didn't think about it enough.

So the problem seems to be here:

https://github.com/vuejs/vue-next/blob/0e5a3c47a7398dfd0107fccf9b615772dd01aa74/packages/reactivity/src/baseHandlers.ts#L85-L88

We should check for isReadonly here

@LinusBorg LinusBorg added the 🐞 bug Something isn't working label Oct 28, 2020
@LinusBorg LinusBorg changed the title readonly(["a", "b", "c"]) Collected by dependency readonly(["a", "b", "c"]).includes() tracks dependencies even though array is nonreactive Oct 28, 2020
@LinusBorg
Copy link
Member

Thanks for reporting, tracked it down and sent a PR.

Apologies for my initial stubborness.

@LinusBorg LinusBorg added the has PR A pull request has already been submitted to solve the issue label Oct 28, 2020
@LinusBorg LinusBorg removed their assignment Oct 28, 2020
@lixiaofa
Copy link
Author

Thank you for answering my questions

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
has PR A pull request has already been submitted to solve the issue 🐞 bug Something isn't working scope: reactivity
Projects
None yet
2 participants