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

[Vue warn] Computed is still dirty after getter evaluation #10341

Closed
csicky opened this issue Feb 14, 2024 · 22 comments
Closed

[Vue warn] Computed is still dirty after getter evaluation #10341

csicky opened this issue Feb 14, 2024 · 22 comments

Comments

@csicky
Copy link

csicky commented Feb 14, 2024

Vue version

3.4.19

Link to minimal reproduction

Steps to reproduce

After the last update I get this warning, but as far as I know I don't have side effects in the computed properties, eslint would warn me about this.
If there are such side effects, the warning is not helping me find them. To make this warning useful it would need to say the name of the computed property. As it is now, it is more just yellow noise in the console.

What is expected?

To either not show warnings, or if it does, to give me the exact name of the computed property and if possible why it thinks it is dirty.

What is actually happening?

I get warnings I can't act upon.

System Info

System:
    OS: Windows 10 10.0.19045
    CPU: (16) x64 AMD Ryzen 7 3700X 8-Core Processor
    Memory: 10.28 GB / 31.93 GB
  Binaries:
    Node: 20.11.0 - C:\Program Files\nodejs\node.EXE
    npm: 10.2.4 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Edge: Chromium (121.0.2277.112)
    Internet Explorer: 11.0.19041.3636
  npmPackages:
    vue: ^3.4.19 => 3.4.19

Any additional comments?

I am not sure if the computed properties it complains about are actual computed declared in the component or state data imported with mapState or mapGetters

@vladyslav-mikhieiev
Copy link

I have a place where the alert points to a ref and this ref can be true or false. However, the component does not have computed. Ref is used to display markup after loading data in onMounted

@solaris7791
Copy link

it would be nice if the warning from commit f7ba97f
could be disabled or at least made configurable

It occurs in common scenarios where deeper getter chains may retrigger effects.
As long its no endless recursion (which anyway cause a warning after 100 iterations) it really does not help.

@Doctor-wu
Copy link
Member

Hey guys, I'm attempting to add some information about this warning, what information do you think is helpful?

@Doctor-wu
Copy link
Member

BTW, I found usePiniaStore in computed will cause this warning, in this case, extract the store to outside can solve this.
CleanShot 2024-02-15 at 19 20 01@2x

@csicky
Copy link
Author

csicky commented Feb 15, 2024

Hey guys, I'm attempting to add some information about this warning, what information do you think is helpful?

Hi, it would be helpful to know the name of the computed triggering the warning, also if possible a way to know why the warning was triggered (for example it changed x times in x timespan). I do have in many places computed cascading, like one using up another and on and on, but in my opinion that should not trigger. Also I would like to know if the mapState and mapGetters does trigger this.

@emikoshi
Copy link

Agreed with @solaris7791 and @csicky. More information about the name of the computed and why, or at least being able to have some sort of configuration. I linked a pinia playground here showing a simple example of how mapState is triggering this, the discussion related to this is #10340.

It doesn't seem right to me that using mapState should consistently throw this warning. @Doctor-wu could you provide an example of how you are saying this can be solved?

@SirH3nry
Copy link

BTW, I found usePiniaStore in computed will cause this warning, in this case, extract the store to outside can solve this. CleanShot 2024-02-15 at 19 20 01@2x

Sorry, what do you mean by extract it to outside?

@Doctor-wu
Copy link
Member

Doctor-wu commented Feb 15, 2024

BTW, I found usePiniaStore in computed will cause this warning, in this case, extract the store to outside can solve this. CleanShot 2024-02-15 at 19 20 01@2x

Sorry, what do you mean by extract it to outside?
@SirH3nry
CleanShot 2024-02-15 at 23 52 10@2x

@SirH3nry
Copy link

SirH3nry commented Feb 15, 2024

BTW, I found usePiniaStore in computed will cause this warning, in this case, extract the store to outside can solve this. CleanShot 2024-02-15 at 19 20 01@2x

Sorry, what do you mean by extract it to outside?

CleanShot 2024-02-15 at 23 52 10@2x

Thanks!
Side question: is it even valid for the warning to fire in this scenario? Does using the store this way align with pinia's or vue3's code design goals? I guess my concern is code cleanliness and that this feels kinda band-aid-y

@Doctor-wu
Copy link
Member

Doctor-wu commented Feb 15, 2024

BTW, I found usePiniaStore in computed will cause this warning, in this case, extract the store to outside can solve this. CleanShot 2024-02-15 at 19 20 01@2x

Sorry, what do you mean by extract it to outside?

CleanShot 2024-02-15 at 23 52 10@2x

Thanks! Side question: is it even valid for the warning to fire in this scenario? Does using the store this way align with pinia's or vue3's code design goals? I guess my concern is code cleanliness and that this feels kinda band-aid-y

I'm still investigating this, it seems useStore read some reactive data and mutated those data, which made computed dirty. The worse news is mapState or mapGetters used useStore internally which is hard to avoid😢, still working on it

@jsormaz
Copy link

jsormaz commented Feb 15, 2024

BTW, I found usePiniaStore in computed will cause this warning, in this case, extract the store to outside can solve this.

I have a similar usecase, except my Pinia store is dynamically determined within the computed, so I can't really extract it outside the computed

do you have any recommendations?

My thought was to maybe do something like this:

component.vue:

<script setup>
import { pauseTracking, resetTracking } from "@vue/reactivity";

const item = computed(() => {
          const itemType = someDynamicItemType // this comes from an API response or some other dependency
          const itemId = someDynamicItemId

          pauseTracking()
          const store = piniaStoreFactory(itemType)
          resetTracking()

          return store.item(itemId)
     }
)
</script>

data-store.js:

function genericStoreFuncGenerator(itemType) {
     return () => {
          //generic store with some tailoring based on the item type, setup an internal axios instance to a specific API endpoint etc...
         const data = ref({})
         const item = computed(()=> (id)=> data.value[id])

         return {data, item}
     }
}

function piniaStoreFactory(itemType) {
     const useDynamicStore = defineStore(itemType, genericStoreFuncGenerator(itemType))
     return useDynamicStore()
}

Does this seem like a bad idea? I don't really understand the implications of pauseTrack + resetTracking

@lehni
Copy link
Contributor

lehni commented Feb 16, 2024

Hey guys, I'm attempting to add some information about this warning, what information do you think is helpful?

@Doctor-wu I've patched reactivity.esm-bundler.js and runtime-core.esm-bundler.js locally to include the key in the call to computed(), and pass it all the way through to the warning message. It really helps with understanding which part of the code causes the warning. This would be a useful change.

@Doctor-wu
Copy link
Member

Hey guys, I'm attempting to add some information about this warning, what information do you think is helpful?

@Doctor-wu I've patched reactivity.esm-bundler.js and runtime-core.esm-bundler.js locally to include the key in the call to computed(), and pass it all the way through to the warning message. It really helps with understanding which part of the code causes the warning. This would be a useful change.

You mean the getter?

@lehni
Copy link
Contributor

lehni commented Feb 17, 2024

@Doctor-wu yes

@Drumstix42
Copy link

I don't think this warning makes any sense for simple use cases. As per this comment:

I linked a pinia playground here showing a simple example of how mapState is triggering this, the discussion related to this is #10340.

You can have a simple Pinia store with only state and no computed values, and you stilll get the Pinia warning.

Shouldn't this be considered a bug with Pinia?

@Doctor-wu
Copy link
Member

I don't think this warning makes any sense for simple use cases. As per this comment:

I linked a pinia playground here showing a simple example of how mapState is triggering this, the discussion related to this is #10340.

You can have a simple Pinia store with only state and no computed values, and you stilll get the Pinia warning.

Shouldn't this be considered a bug with Pinia?

Agreed, that's because mapState use useStore internally and useStore has side-effect.

@SirH3nry
Copy link

Started a discussion on Pinia's side. Not sure if creating a bug is appropriate.
vuejs/pinia#2585

@Doctor-wu
Copy link
Member

Doctor-wu commented Feb 23, 2024

@Doctor-wu yes

#10386 Here's a PR for this
A sample for warning👇🏻, click on the function can jump to the source in devtools
CleanShot 2024-02-23 at 16 39 53@2x

@m-shum
Copy link

m-shum commented Feb 23, 2024

I'm getting this warning without pinia in a component – occurs for me whenever I add a nuxtUI element.

@RaynisDev
Copy link

I'm getting this warning without pinia in a component – occurs for me whenever I add a nuxtUI element.

i started a nuxt3 project from scratch to investigate this warning.. just v-calendar module installed and.... warning flood

@SirH3nry
Copy link

@Doctor-wu yes

#10386 Here's a PR for this A sample for warning👇🏻, click on the function can jump to the source in devtools CleanShot 2024-02-23 at 16 39 53@2x

@Doctor-wu I haven't been getting this warning anymore since upgrading to x.20. Did something change?

@Doctor-wu
Copy link
Member

@Doctor-wu yes

#10386 Here's a PR for this A sample for warning👇🏻, click on the function can jump to the source in devtools CleanShot 2024-02-23 at 16 39 53@2x

@Doctor-wu I haven't been getting this warning anymore since upgrading to x.20. Did something change?

that warning doesn't display by default in the x.20 version

mgineer85 added a commit to photobooth-app/photobooth-frontend that referenced this issue Feb 27, 2024
OnlyWick pushed a commit to OnlyWick/core that referenced this issue Feb 27, 2024
Now can be enabled with app.config.warnRecursiveComputed option.

close vuejs#10341
@github-actions github-actions bot locked and limited conversation to collaborators Mar 12, 2024
gennitdev added a commit to gennit-project/multiforum-frontend that referenced this issue Mar 26, 2024
lynxlangya pushed a commit to lynxlangya/core that referenced this issue May 30, 2024
Now can be enabled with app.config.warnRecursiveComputed option.

close vuejs#10341
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests