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: Add a check for isEqual to reduce duplicate queries #332

Merged
merged 1 commit into from
Oct 26, 2023
Merged

Conversation

zhen0
Copy link
Member

@zhen0 zhen0 commented Oct 26, 2023

@zhen0 zhen0 requested a review from a team as a code owner October 26, 2023 16:35
@pleek91
Copy link
Collaborator

pleek91 commented Oct 26, 2023

This fixes a reactivity issue where modifying the query at all would trigger reactive effects for anything using useRouteQueryParam even if the value for a specific param didn't change.

@collincchoy
Copy link
Contributor

collincchoy commented Oct 26, 2023

This fixes a reactivity issue where modifying the query at all would trigger reactive effects for anything using useRouteQueryParam even if the value for a specific param didn't change.

Hmm trying to reproduce this on main and not seeing it. Been trying a few things but based on the above quote, I'd expect the following to trigger a console log on every button click?

<template>
  <button @click="testing">click me</button>
</template>

<script setup lang="ts">
  const tab = useRouteQueryParam('tab')
  watch(param1, () => {
    console.log('tab changed', tab.value)
  })

  const otherParam = useRouteQueryParam('otherParam')
  function testing(): void {
    otherParam.value = randomId()  // different uuid everytime, but not altering the tab param
  }
</script>

I also tried updating the query directly via useRouteQuery and even directly through vue router...

I also tried updating the tab param directly to the value it's already at and that didn't trigger effects either.

@collincchoy
Copy link
Contributor

collincchoy commented Oct 26, 2023

ah okay so after @zhen0 showed me something, it feels like the bug is actually something like updating the a query param via useRouteQueryParam makes unnecessary updates in between.

<template>
  <button type="button" @click="testing">click me</button>
</template>
<script setup lang="ts">
  const tab = useRouteQueryParam('tab')
  watch(tab, () => {
    console.log('tab changed', tab.value)
  })

  function testing(): void {
    tab.value = randomId()
  }
</script>

I can reproduce that clicking that^ causes console output like

tab changed bd123e4b-75e2-41bd-bd20-39c53a246fe7. ---- (new)
tab changed 93b2ba30-2797-4c78-ab38-7d987913bc05 ---- (old)
tab changed bd123e4b-75e2-41bd-bd20-39c53a246fe7 ---- (new)

on every button click instead of just the expected: tab changed bd123e4b-75e2-41bd-bd20-39c53a246fe7

Copy link
Contributor

@collincchoy collincchoy left a comment

Choose a reason for hiding this comment

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

I'm a little concerned that this feels like a bandaid where the problem is a little lower down. From what I can tell, the issue was actually that format.set(query, value) in the computed's setter is triggering updates to more things than just the query which is what's causing reactive effects to trigger multiple times. This PR works because it breaks down the computed into smaller pieces which explicitly should only change when the query changes - thus losing the dependency tracking on whatever things inside of format are changing and causing triggers.

That being said, this does seem to fix things so I don't want to block this per se - I just wonder if other things are affected by this issue that this change won't also resolve.

@zhen0 zhen0 merged commit 3aac7c0 into main Oct 26, 2023
@zhen0 zhen0 deleted the duplicates branch October 26, 2023 20:20
This was referenced Oct 26, 2023
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