Skip to content

refactor: vue implementation #85

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

teleskop150750
Copy link

@teleskop150750 teleskop150750 commented May 15, 2025

refactor: vue implementation

Copy link

changeset-bot bot commented May 15, 2025

⚠️ No Changeset found

Latest commit: 492596d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@wobsoriano
Copy link
Contributor

wobsoriano commented May 15, 2025

Hi @teleskop150750! Curious as to what's wrong with the current one? I tried to keep it 1:1 with the React implementation (including the use of @tanstack/store) and make it idiomatic by using computed instead of setting refs inside watchers.

Happy to know if there's a specific issue or limitation with the current implementation 🫡

Comment on lines 54 to 68
const unsubDerivedState = derivedState.subscribe(() => {
const newValue = derivedState.state
if (shallow(stateRef, newValue)) return

stateRef = newValue
state.value = newValue
})

const unsubDerivedArray = derivedArray.subscribe(() => {
const newValue = derivedArray.state
if (shallow(dataRef, newValue)) return

dataRef = newValue
data.value = newValue
})
Copy link
Contributor

Choose a reason for hiding this comment

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

I think @tanstack/store handles this already?

Copy link
Author

Choose a reason for hiding this comment

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

Vue and tanstack libraries unfortunately have a very difficult fate (((

TanStack/store#179

  1. the first reason is the reactors of the library @tanstack/store (drop vue2, refactor unsub)
  2. the current implementation of @tanstack/store uses a deep link, which makes no sense since the entire object gets replaced. Unfortunately, vue primitives are woefully behind solidjs.

https://github.com/TanStack/store/blob/cf37b85ddecdcb6f52ad930dcd53e294fb4b03a7/packages/solid-store/src/index.tsx#L31

Solid js uses reconcile for fine-grained updating. If desired, you can try writing an analogue for vue

Comment on lines 28 to 35
const results = shallowRef() as Ref<
ReturnType<typeof compileQuery<TResultContext>>[`results`]
>

const state = shallowRef() as Ref<
Map<string, ResultsFromContext<TResultContext>>
>
const data = shallowRef() as Ref<Array<ResultsFromContext<TResultContext>>>
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason on why we are not passing the types to the primitive instead?

like

const results = shallowRef<ReturnType<typeof compileQuery<TResultContext>>[`results`]>()

Copy link
Author

@teleskop150750 teleskop150750 May 15, 2025

Choose a reason for hiding this comment

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

For

const foo = shallowRef<T>()
// foo: Ref<T | undefined>

The output will actually be Ref <T | undefined> because we didn't pass a value

@teleskop150750
Copy link
Author

teleskop150750 commented May 15, 2025

Hi @teleskop150750! Curious as to what's wrong with the current one? I tried to keep it 1:1 with the React implementation (including the use of @tanstack/store) and make it idiomatic by using computed instead of setting refs inside watchers.

Happy to know if there's a specific issue or limitation with the current implementation 🫡

My implementation still follows the react code. The only difference is that I don't use vue-store

For optimization purposes, 1 effect is used instead of 5 (1-watch and 4 computed)

Because of some stereotypes and limitations of Api ref, people are afraid to use getter functions.
Link
It seems to me that in this line the extra computed does not optimize anything, but is used only to be in the style of vue api.
An easier way would be to simply return the function

{
  collection: () => compiledQuery.value.results
}

Changed input options interface to.

deps: () => Array<unknown> = () => []

I wrote about it in great detail here

vuejs/rfcs#747

It seems to me that deps are not needed at all, because when calling queryFn, all reactive variables will automatically fall into dependencies. For more detailed configuration, we need support for the untrack method in vue or use watch

@wobsoriano
Copy link
Contributor

Hi @teleskop150750! Curious as to what's wrong with the current one? I tried to keep it 1:1 with the React implementation (including the use of @tanstack/store) and make it idiomatic by using computed instead of setting refs inside watchers.
Happy to know if there's a specific issue or limitation with the current implementation 🫡

My implementation still follows the react code. The only difference is that I don't use vue-store

For optimization purposes, 1 effect is used instead of 5 (1-watch and 4 computed)

Because of some stereotypes and limitations of Api ref, people are afraid to use getter functions. Link It seems to me that in this line the extra computed does not optimize anything, but is used only to be in the style of vue api. An easier way would be to simply return the function

{
  collection: () => compiledQuery.value.results
}

Changed input options interface to.

deps: () => Array<unknown> = () => []

I wrote about it in great detail here

vuejs/rfcs#747

It seems to me that deps are not needed at all, because when calling queryFn, all reactive variables will automatically fall into dependencies. For more detailed configuration, we need support for the untrack method in vue or use watch

Thanks for the detailed explanation and the references!

From what I understand, your refactor aims to optimize for certain edge cases and performance, but it seems to come at the cost of clarity and idiomatic Vue code. Is that correct?

re: deps array - I think this feature is similar to Nuxt's useAsyncData watch option for cases where you want to refresh the query based on other reactive variables that aren’t directly referenced in queryFn

@teleskop150750
Copy link
Author

deps array

There is no point in this because the result of the query will be the same
I think the reason is that react requires explicit transmission of all dependencies

@teleskop150750
Copy link
Author

@wobsoriano Unfortunately I couldn't figure out where it came from.

newQuery.start()

throw new Error(`Query is stopped`)

@teleskop150750
Copy link
Author

teleskop150750 commented May 15, 2025

idiomatic Vue code

Example vueuse

@teleskop150750
Copy link
Author

I can agree with you that computed can be useful for lazy invocation.

@teleskop150750
Copy link
Author

teleskop150750 commented May 17, 2025

@wobsoriano

look at the experimental implementation with computed, I return the entire ref so that the subscription is not created every time the value is updated

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.

2 participants