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

feat(runtime-core): v-memo allow for dynamic / external array #4255

Merged
merged 3 commits into from
Aug 9, 2021

Conversation

lidlanca
Copy link
Contributor

@lidlanca lidlanca commented Aug 4, 2021

add ability to use external array as source for v-memo

<div v-memo="arr">

</div>

<script setup>
   let arr = reactive([x.value, y.value])

</script>

@posva
Copy link
Member

posva commented Aug 5, 2021

In what case will this be useful? If the array is external, it's much easier to make it not-fixed size and break it. The utility of v-memo is to base itself on things used to render, it should not be able to use properties that might not be used during rendering. Allowing this approach will make it much harder to debug

@lidlanca
Copy link
Contributor Author

lidlanca commented Aug 5, 2021

memoization logic is not dependent or restricted by what is rendering nor should it be.

and complex non debuggable logic already possible in template.

<div v-memo="[count < 3 ? a : b ,  count >=3 && < 3 && b ? b : 0  ]"></div>

having logic in code is arguably more debuggable and testable then being in the template, for the non trivial use cases

the declarative approach has its place, but it is not the only approach possible.

also if one wants dynamic array, sky is the limit.

<div v-memo=[...items]></div>

@posva
Copy link
Member

posva commented Aug 5, 2021

The array should not be dynamic. It should be fixed length.

What are some the realistic complex use cases you thinking about? The one with ternaires doesn't seem realistic

Have you read the PR in the docs repo containing docs for v-memo?

@lidlanca
Copy link
Contributor Author

lidlanca commented Aug 5, 2021

I have read the document, after you pointed out its existence.

The document only considers one actual use case for this api. ( v-for optimization )
and we yet to discover more useful uses for the api

https://github.com/vuejs/docs/blob/55505e758466a43e7e9c6e406dc2be4632604f71/src/api/directives.md

The directive expects a fixed-length array of dependency values to compare for the memoization

it is an expectation for the current diff logic to work, the requirement does not contradict passing a fixed size array from script section.

this feature also seem to cater for advance use cases, with the onus on the developer to correctly pass and construct the right value for the memoization.

The PR account for dependency size change as a dependency change, even if the most likely use case is fixed size dependencies.
there is nothing preventing someone from using non fixed size array, as shown in my last message.

since there are plenty of possible work around, I consider this pr as nice to have.
but non blocking.

@posva
Copy link
Member

posva commented Aug 5, 2021

I didn't say it was blocking only asked what are the real use cases.

Note that widening an api can make it easier to mess up for the user and harder to debug. On the other hand, limiting its usage makes it easier to detect errors and point the user in the right direction.

Real use cases are also interesting for documentation purposes and to know if v-memo usage makes sense or not

@lidlanca
Copy link
Contributor Author

lidlanca commented Aug 5, 2021

I understand your rational, having dynamic size memo should be considered experimental, we might not be able to predict its impact without some more community feedback.

we don't have to support this use case officially until further feedback, in the meantime there are plenty of ways
to introduce dynamic array to v-memo, so we might as well allow for some experimentation and basic functionality of invalidating the memo on size change.

passing the dependencies from code on the other hand is a coincidental limitation due to an array declared in the template always being a copy. and an array from code being a reference resulting in diffing the same object ( hence the shallow copy in the pr).

it is currently harder for me to experiment and discover good use cases for v-memo
due to pending open bugs ( #4253, #4246)

but to be honest I don't want to look for a advance use cases, I am sure those will come up organically by the community, I rather focus on strengthening the api to allow for more advance use cases by strengthening the basic functionality, which is already supported one way or another.

I believe there can be many creative usages for v-memo, as it can be used as a mechanism for view update denouncing

for example one can use v-memo to debounce a chart update based on a timer

@posva
Copy link
Member

posva commented Aug 5, 2021

I am sure those will come up organically by the community

Not always but all my points still remain. For instance the use case you cited can already be handled with a customRef to defer triggers.

Anyway, we don't add new apis just because it's possible. If it was like that libraries size would've grow.

@lidlanca
Copy link
Contributor Author

lidlanca commented Aug 5, 2021

Not always
where is the v-memo rfc, I am curious to see why there was no valuable feedback from the community.

there are many points we discussed some relevant some less, my main point that might have been lost in our informative discussion is that if I can do v-memo=[...arr] v-memo=getMeMyArrayAndClone() v-memo=arr.filter(fn) we might as well be able to just pass v-memo=arr.

The size detection was just a defensive guard for what is already possible.

Its seem like its just you and me ping-ponging , perhaps this should be closed until the feature is more known and used and more feedback is gathered

appreciate the time you took to go over this pr, and explain your points.

@posva
Copy link
Member

posva commented Aug 5, 2021

We haven't even discussed anything yet 😅
The first and most important question of any feature request remains: what is the use case? This is what we always ask, it's nothing personal. Maybe reading the rfcs repo readme gives you a more detailed explanation of why we do this...

@yyx990803
Copy link
Member

The cost seems reasonable to support edge cases. Note this doesn't affect codegen of v-memo + v-for, which is ok because in that case the memo array should be a new array for each item anyway.

@yyx990803 yyx990803 closed this Aug 9, 2021
@yyx990803 yyx990803 reopened this Aug 9, 2021
@yyx990803 yyx990803 merged commit 6779bff into vuejs:master Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants