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: @pinia/nuxt breaks createTestingPinia #2578

Closed
wants to merge 1 commit into from

Conversation

OnlyWick
Copy link
Contributor

fix #2555

Copy link

netlify bot commented Feb 13, 2024

Deploy Preview for pinia-playground canceled.

Name Link
🔨 Latest commit 235483b
🔍 Latest deploy log https://app.netlify.com/sites/pinia-playground/deploys/65cbb3ba42e3ad0008abddab

Copy link

netlify bot commented Feb 13, 2024

Deploy Preview for pinia-official canceled.

Name Link
🔨 Latest commit 235483b
🔍 Latest deploy log https://app.netlify.com/sites/pinia-official/deploys/65cbb3ba939a5b000856299b

@OnlyWick OnlyWick marked this pull request as draft February 13, 2024 09:30
@OnlyWick
Copy link
Contributor Author

The reason is that there is a conflict between the pinia instance created by @pinia/nuxt and createTestingPinia. Ultimately, the pinia instance created by @pinia/nuxt takes precedence, causing the tests to fail.

Comment on lines 109 to 112
const app: App = (globalThis as any).__unctx__.get('nuxt-app').tryUse().vueApp
const symbols = Object.getOwnPropertySymbols(app._context.provides)
const nuxtPinia: Pinia = app._context.provides[symbols[0]]
const pinia = nuxtPinia ?? createPinia()
Copy link
Member

Choose a reason for hiding this comment

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

Thanks but not only we always want to create a new pinia instance here, this code is relying on internals and globals that are specific to nuxt. This will break in any other scenario

Copy link
Contributor Author

@OnlyWick OnlyWick Feb 13, 2024

Choose a reason for hiding this comment

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

This is temporary fix code. You are right, this code lacks flexibility. However, there is another issue where addPlugin from @pinia/nuxt creates a new pinia instance, which conflicts with the creation of createTestingPinia. But the logic of @nuxt/pinia is reasonable, and I believe it should not be modified.

Copy link
Member

Choose a reason for hiding this comment

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

It's okay to adapt @nuxt/pinia to work with the test environment. It's really crucial to always create a new pinia instance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it.

@OnlyWick OnlyWick requested a review from posva February 13, 2024 12:07
@OnlyWick OnlyWick force-pushed the fix-#2555 branch 2 times, most recently from d88b1e8 to 5ec01cf Compare February 13, 2024 18:22
@OnlyWick OnlyWick marked this pull request as ready for review February 13, 2024 18:27
@OnlyWick
Copy link
Contributor Author

Sad :-(. It's diffcult to me. I gave up.

I have subscribed to this issue, so please go ahead and fix it. I would like to learn how it is fixed.

@posva
Copy link
Member

posva commented Feb 14, 2024

No worries, thanks for the try!

@posva posva closed this Feb 14, 2024
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.

Pinia Nuxt module breaks the createTestingPinia helper in tests
2 participants