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(vue-query): use shallowReactive and shallowReadonly to prevent performance slow down for large dataset #7733

Merged
merged 3 commits into from
Sep 5, 2024

Conversation

wns3645
Copy link
Contributor

@wns3645 wns3645 commented Jul 13, 2024

Use shallowReactive and shallowReadonly instead of reactive and readonly

reactive and readonly uses deep conversion which affects all nested properties. Therefore, performance slows when the data is a large array of deeply nested objects.

shallowReactive is enough because we need reactivity for only shallow properties(i.e., properties at the first depth).
Although shallowReadonly cannot strictly prevent mutating deeply nested properties, it would be enough as ref values returned from third-party composables are expected not to change directly.

@wns3645 wns3645 changed the title fix(vue-query): use shallowReactive and shallowReadonly to prevent pe… fix(vue-query): use shallowReactive and shallowReadonly to prevent performance slow down for large dataset Jul 13, 2024
@wns3645 wns3645 marked this pull request as ready for review July 13, 2024 02:04
Copy link

nx-cloud bot commented Jul 16, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit a3f15f3. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

Copy link

pkg-pr-new bot commented Jul 16, 2024

Open in Stackblitz

More templates

@tanstack/angular-query-devtools-experimental

pnpm add https://pkg.pr.new/@tanstack/angular-query-devtools-experimental@7733

@tanstack/angular-query-experimental

pnpm add https://pkg.pr.new/@tanstack/angular-query-experimental@7733

@tanstack/query-async-storage-persister

pnpm add https://pkg.pr.new/@tanstack/query-async-storage-persister@7733

@tanstack/eslint-plugin-query

pnpm add https://pkg.pr.new/@tanstack/eslint-plugin-query@7733

@tanstack/query-broadcast-client-experimental

pnpm add https://pkg.pr.new/@tanstack/query-broadcast-client-experimental@7733

@tanstack/query-core

pnpm add https://pkg.pr.new/@tanstack/query-core@7733

@tanstack/query-devtools

pnpm add https://pkg.pr.new/@tanstack/query-devtools@7733

@tanstack/query-persist-client-core

pnpm add https://pkg.pr.new/@tanstack/query-persist-client-core@7733

@tanstack/query-sync-storage-persister

pnpm add https://pkg.pr.new/@tanstack/query-sync-storage-persister@7733

@tanstack/react-query

pnpm add https://pkg.pr.new/@tanstack/react-query@7733

@tanstack/react-query-next-experimental

pnpm add https://pkg.pr.new/@tanstack/react-query-next-experimental@7733

@tanstack/react-query-devtools

pnpm add https://pkg.pr.new/@tanstack/react-query-devtools@7733

@tanstack/react-query-persist-client

pnpm add https://pkg.pr.new/@tanstack/react-query-persist-client@7733

@tanstack/solid-query

pnpm add https://pkg.pr.new/@tanstack/solid-query@7733

@tanstack/solid-query-devtools

pnpm add https://pkg.pr.new/@tanstack/solid-query-devtools@7733

@tanstack/solid-query-persist-client

pnpm add https://pkg.pr.new/@tanstack/solid-query-persist-client@7733

@tanstack/svelte-query

pnpm add https://pkg.pr.new/@tanstack/svelte-query@7733

@tanstack/svelte-query-devtools

pnpm add https://pkg.pr.new/@tanstack/svelte-query-devtools@7733

@tanstack/svelte-query-persist-client

pnpm add https://pkg.pr.new/@tanstack/svelte-query-persist-client@7733

@tanstack/vue-query

pnpm add https://pkg.pr.new/@tanstack/vue-query@7733

@tanstack/vue-query-devtools

pnpm add https://pkg.pr.new/@tanstack/vue-query-devtools@7733

commit: a3f15f3

Copy link

codecov bot commented Jul 16, 2024

Codecov Report

Attention: Patch coverage is 72.72727% with 6 lines in your changes missing coverage. Please review.

Project coverage is 71.51%. Comparing base (79e5202) to head (a3f15f3).
Report is 136 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #7733       +/-   ##
===========================================
+ Coverage   44.56%   71.51%   +26.95%     
===========================================
  Files         185       19      -166     
  Lines        7062      481     -6581     
  Branches     1552      136     -1416     
===========================================
- Hits         3147      344     -2803     
+ Misses       3552      107     -3445     
+ Partials      363       30      -333     
Components Coverage Δ
@tanstack/angular-query-devtools-experimental ∅ <ø> (∅)
@tanstack/angular-query-experimental ∅ <ø> (∅)
@tanstack/eslint-plugin-query ∅ <ø> (∅)
@tanstack/query-async-storage-persister ∅ <ø> (∅)
@tanstack/query-broadcast-client-experimental ∅ <ø> (∅)
@tanstack/query-codemods ∅ <ø> (∅)
@tanstack/query-core ∅ <ø> (∅)
@tanstack/query-devtools ∅ <ø> (∅)
@tanstack/query-persist-client-core ∅ <ø> (∅)
@tanstack/query-sync-storage-persister ∅ <ø> (∅)
@tanstack/react-query ∅ <ø> (∅)
@tanstack/react-query-devtools ∅ <ø> (∅)
@tanstack/react-query-next-experimental ∅ <ø> (∅)
@tanstack/react-query-persist-client ∅ <ø> (∅)
@tanstack/solid-query ∅ <ø> (∅)
@tanstack/solid-query-devtools ∅ <ø> (∅)
@tanstack/solid-query-persist-client ∅ <ø> (∅)
@tanstack/svelte-query ∅ <ø> (∅)
@tanstack/svelte-query-devtools ∅ <ø> (∅)
@tanstack/svelte-query-persist-client ∅ <ø> (∅)
@tanstack/vue-query 71.51% <72.72%> (+0.08%) ⬆️
@tanstack/vue-query-devtools ∅ <ø> (∅)

@DamianOsipiuk
Copy link
Contributor

I'm fine with shallowReactive, but shallowReadonly would open up a footgun which was prevented multiple times alredy.

People just tried to modify returned data and opened bug reports that they can't.

Maybe we could achieve this conditionally by introducing some kind of removeSafeguardsIKnowWhatImDoing prop that would modify the behaviour and return shallow read-only version.

@wns3645
Copy link
Contributor Author

wns3645 commented Jul 29, 2024

@DamianOsipiuk
I understand your concern about readonly.

In react-query, the data object returned by useQuery is also not immutable. Issues about mutating properties in data directly can occur in react-query, too(let me know if i'm wrong). I don't know the exact reason that react-query doesn't provide deeply nested readonly data object, but I think always providing readonly object seems not feasible in general.
My opinion is that preventing only directly mutating data.value via shallowReadonly can be sufficient to avoid unexpected triggering reactivity effects while providing the return value of frozen objects similar to react-query.

Otherwise, providing kind of removeSafeguardsIKnowWhatImDoing option also make sense. For example, in vus-use, some async state composables provide shallow option that can control the return value will be shallow or not. (default is shallow: true because of the performance for large dataset)

@teleskop150750
Copy link

teleskop150750 commented Jul 29, 2024

it would be nice to mark readonly only in development mode

an example from vue props

Link

@teleskop150750
Copy link

will there be an advantage if you immediately use shallowRef, without casting toRefs?

@teleskop150750
Copy link

teleskop150750 commented Jul 30, 2024

@wns3645

It can also be used in useMutationState

import { hasChanged } from '@vue/shared'
import { type Ref, customRef } from 'vue'

type Dispatch<A> = (value: A) => void
type SetStateAction<S> = S | ((prevState: S) => S)
/**
 * Returns a stateful value, and a function to update it.
 */
export function useState<S>(initialState: S | (() => S)): [Readonly<Ref<S>>, Dispatch<SetStateAction<S>>]
// convenience overload when first argument is omitted
/**
 * Returns a stateful value, and a function to update it.
 */
export function useState<S = undefined>(): [Readonly<Ref<S | undefined>>, Dispatch<SetStateAction<S | undefined>>]

export function useState(initialState?: unknown) {
  let value = typeof initialState === 'function' ? initialState() : initialState
  let _trigger = () => {}

  const state = customRef((track, trigger) => {
    _trigger = trigger
    return {
      get() {
        track()
        return value
      },
      set() {
        console.error('state is readonly')
      },
    }
  })


  const setState = (newValue: unknown) => {
    const _newValue = typeof newValue === 'function' ? newValue(value) : newValue

    if (hasChanged(value, _newValue)) {
      value = _newValue
      _trigger()
    }
  }

  return [state, setState]
}
const [state, setState] = useState<number>()

@wns3645
Copy link
Contributor Author

wns3645 commented Aug 3, 2024

will there be an advantage if you immediately use shallowRef, without casting toRefs?

In useBaseQuery and useMutation, using shallowReactive and toRefs make it easy to convert each property(data, isLoading, isFetching, ...) of return value of observer.getCurrentResult() being ref.

It can also be used in useMutationState

@teleskop150750
Applied shallowRef and shallowReadonly to useMutationState, too.-> dcd4e58

@teleskop150750
Copy link

teleskop150750 commented Aug 3, 2024

will there be an advantage if you immediately use shallowRef, without casting toRefs?

In useBaseQuery and useMutation, using shallowReactive and toRefs make it easy to convert each property(data, isLoading, isFetching, ...) of return value of observer.getCurrentResult() being ref.

It can also be used in useMutationState

@teleskop150750 Applied shallowRef and shallowReadonly to useMutationState, too.-> dcd4e58

why didn't you want to use readonly only in dev mode?

@wns3645
Copy link
Contributor Author

wns3645 commented Aug 29, 2024

why didn't you want to use readonly only in dev mode?

@teleskop150750
Using deep readonly in dev mode also causes performance issues when we want to test the large dataset in dev mode. I think that's why the vue core example you gave also uses shallow readonly.
Using shallow readonly also in production mode may not be harmful to the performance.

@DamianOsipiuk , any thoughts on this PR? Should be closed or can be complemented?

@teleskop150750
Copy link

In the examples given, vue/core, I also meant shallowReadonly. Still, I'm inclined to make it only in dev mode. The vue/core example is a good argument. In any case, shallowReadonly will not eliminate some violations of the rules specified in the documentation. Therefore, it makes no more sense to send it into production. Some tanstack/query implementations in other frameworks do not worry about this and leave it to the developer's responsibility.

@wns3645
Copy link
Contributor Author

wns3645 commented Aug 30, 2024

@teleskop150750 Yes, I agree with you about using shallowReadonly only in dev mode like vue/core example.(I thought about using deep readonly 😅)

Some tanstack/query implementations in other frameworks do not worry about this and leave it to the developer's responsibility.

Agree, this is the reason that I think we can remove deep readonly(and also shallow readonly in prod you suggested) in vue-query code base

@DamianOsipiuk
Copy link
Contributor

I would still like to get shallow option and retain deep readonly by default.
To prevent inexperienced devs mutating the cache by example using v-model on part of the data.

Stripping readonly completely from prod bundle seems ok.

@teleskop150750
Copy link

teleskop150750 commented Aug 30, 2024

example:

if you use shallowRef directly.

// packages/vue-query/src/useBaseQuery.ts

import type { DeepReadonly } from 'vue-demi'

export function useBaseQuery(): DeepReadonly<UseBaseQueryReturnType<TData, TError>>

// 105 line
const observer = new Observer(client, defaultedOptions.value)

type WrapInRef<T> = {
  [K in keyof T]: T[K] extends Function ? T[K] : Ref<T[K]>;
};

type State = WrapInRef<QueryObserverResult<TData, TError>>;

const rawState = observer.getCurrentResult()

const state = rawStateToState(rawState)

function rawStateToState(payload: QueryObserverResult<TData, TError>): State {
  const ret: any = {}

  for (const key of Object.keys(payload) as Array<keyof QueryObserverResult<TData, TError>>) {
    if (typeof payload[key] === 'function') {
      ret[key] = payload[key]
    } else {
      ret[key] = shallowRef(payload[key])
    }
  }

  return ret
}

let unsubscribe = () => {
  // noop
}

// .....

const object = state as unknown as UseBaseQueryReturnType<TData, TError>

object.suspense = suspense
object.refetch = refetch

return object as DeepReadonly<UseBaseQueryReturnType<TData, TError>>
function updateQueryState(payload: State, update: QueryObserverResult<TData, TError>) {
  for (const key of Object.keys(update) as Array<keyof QueryObserverResult<TData, TError>>) {
    if (typeof update[key] === 'function') {
      continue
    }
    (payload[key] as any).value = update[key]
  }
}

@DamianOsipiuk
Copy link
Contributor

@wns3645 Could you check if latest package from this PR works well with shallow flag in your project?
pnpm add https://pkg.pr.new/@tanstack/vue-query@7733

@wns3645
Copy link
Contributor Author

wns3645 commented Sep 5, 2024

@DamianOsipiuk
Tested it with shallow: true option in my project and can confirm it works well 👍

@DamianOsipiuk DamianOsipiuk merged commit 9cb5922 into TanStack:main Sep 5, 2024
8 checks passed
@wns3645 wns3645 deleted the fix/vue-query-performance branch September 6, 2024 04:18
@gu-stav
Copy link

gu-stav commented Sep 6, 2024

Thank you for this 🌻

I am wondering if there is a specific reason why the shallow flag hasn't been added to the global VueQueryPluginOptions, so that it can be set for all queries at once?

app.use(VueQueryPlugin, {
  queryClientConfig: {
    defaultOptions: {
      queries: {
        shallow: boolean
      }
    }
  }
)

n0099 added a commit to n0099/open-tbm that referenced this pull request Sep 8, 2024
…nu>`

* fix new page or query won't update menu items by replace the prop `postPages` with `queryParam` for passing it as the `queryKey` of `useApiPosts()` to sync `data` with `pages/posts.vue`
* now will only expand menus of the current and previous cursor when pages get updated as currently it's not possible to move page backwards
@ `<PostNav>`

* now will display `H3Error.toJSON()` & `Error.stack` which is in html string: https://github.com/nuxt/nuxt/blob/fcf023e611efb662124164b5cbcae12503b0ee0a/packages/nuxt/src/core/runtime/nitro/error.ts#L19
* wrapping with a new root level `div.container`
@ error.vue

* partial revert 9046c33 due to TanStack/query#7733 @ `<PostRendererList>`
@ fe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants