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: add lazy option to asyncComputed #247

Merged
merged 6 commits into from
Dec 14, 2020

Conversation

Demivan
Copy link
Contributor

@Demivan Demivan commented Dec 13, 2020

closes #248

@Demivan Demivan changed the title asyncComputed is not lazy test Add lazy option to asyncComputed Dec 14, 2020
@Demivan Demivan changed the title Add lazy option to asyncComputed feat: add lazy option to asyncComputed Dec 14, 2020
@antfu
Copy link
Member

antfu commented Dec 14, 2020

Can you help to resolve the conflicts? Thanks

@patak-dev
Copy link
Member

About the new options params, given that the other two are optional, maybe at one point the three params could be in a single options object:

const asyncComputed( ()=> { ... }, { initial, evaluating, lazy } )

So users are not forced to add undefined twice if they want the default values for that params.

One question about the trick using computed() to defer the evaluation. In the lazy case, the internal ref and watchEffect will end up being created after the component is mounted, no? That means that they will not be in the same effect scope, and there will be a leak on unmount as they will not be stoped. Do this pattern also needs the effectScope PR from @antfu ? Maybe I am misunderstanding how this works, just checking.

@Demivan
Copy link
Contributor Author

Demivan commented Dec 14, 2020

@antfu Fixed merge conflict.

@matias-capeletto I don't like putting initial value into options object. I think this looks better:

const asyncComputed( ()=> { ... }, initial, { evaluating, lazy })

And it should be possible to check if third parameter is ref and support previous api.

const asyncComputed( ()=> { ... }, initial, evaluating)

I'm actually not sure about effect scope. Will try to write test for this.

@antfu
Copy link
Member

antfu commented Dec 14, 2020

@Demivan the API you proposed looks good to me. Can you update this PR to it? Thanks.

The timing problem @matias-capeletto mentioned may cause some edge cases with effectScope, I think maybe we can have a guard flag for watchEffect and make it try on the computed get trigger. As it might be out of the scope of this PR, feel free to do so or I can take care of it later.

@Demivan
Copy link
Contributor Author

Demivan commented Dec 14, 2020

@antfu Updated PR with new API.
Not sure how to deal with described edge case so leaving it to you.

@antfu antfu merged commit 442e5dd into vueuse:master Dec 14, 2020
@antfu
Copy link
Member

antfu commented Dec 14, 2020

Thanks! See 425a8e0

@Demivan Demivan deleted the lazy-async-computed branch December 14, 2020 20:48
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.

asyncComputed is not lazy like computed
3 participants