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

feat: expose providePinia to use multiple root stores #878

Open
wants to merge 3 commits into
base: v2
Choose a base branch
from

Conversation

bodograumann
Copy link
Contributor

Implements #870.

@codecov
Copy link

codecov bot commented Dec 6, 2021

Codecov Report

Merging #878 (ee7911b) into v2 (421082e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##               v2     #878   +/-   ##
=======================================
  Coverage   99.73%   99.73%           
=======================================
  Files           9        9           
  Lines         376      378    +2     
  Branches      100      100           
=======================================
+ Hits          375      377    +2     
  Partials        1        1           
Impacted Files Coverage Δ
packages/pinia/src/rootStore.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eaed48a...ee7911b. Read the comment docs.

@posva
Copy link
Member

posva commented Dec 6, 2021

Thanks! I will have to give this more thought as there might be some edge cases with setActivePinia() and how it is used across stores

@bodograumann
Copy link
Contributor Author

Is there some way I can help with that?
If you have specific scenarios in mind, I could add tests for them.

@posva posva linked an issue Jan 26, 2022 that may be closed by this pull request
@netlify
Copy link

netlify bot commented Feb 18, 2022

Deploy Preview for pinia-official canceled.

Name Link
🔨 Latest commit f8fd0c8
🔍 Latest deploy log https://app.netlify.com/sites/pinia-official/deploys/62456c8704bd690008e83dd6

@posva
Copy link
Member

posva commented Feb 18, 2022

Currently, there are still a few issues:

  • It shouldn't change the activePinia
  • It cannot use plugins (they require the pinia._a property but even if it was set, it would still mean plugins could only be added after calling providePlugin

Overall, I think exposing the symbol and letting the user use at their own risk might be a better idea. Otherwise, a proper solution would be a custom pinia similar to the testing one

@codecov-commenter
Copy link

Codecov Report

Merging #878 (7c3066b) into v2 (43f690f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##               v2     #878   +/-   ##
=======================================
  Coverage   99.73%   99.74%           
=======================================
  Files           9        9           
  Lines         383      385    +2     
  Branches       97       97           
=======================================
+ Hits          382      384    +2     
  Partials        1        1           
Impacted Files Coverage Δ
packages/pinia/src/rootStore.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43f690f...7c3066b. Read the comment docs.

@posva posva added the WIP label Mar 31, 2022
@posva
Copy link
Member

posva commented Mar 31, 2022

@bodograumann would exposing the symbol be enough?

I added a couple of tests to explain what I meant. Maybe createPinia() could detect it's a nested pinia but that would also mean it would add to the library size so it should be really minimal.

There is also a concern about devtools. Right now, they expect no more than one pinia to be created for an app.

@bodograumann
Copy link
Contributor Author

I have a hard time wrapping my head around the whole context right now.
In the feature request some more details are layed out however.
My past self thought that exposing the symbol is good enough.
Also, I’m only using this functionality in the context of storybook+createTestingPinia.
If it helps, I could create an example project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 💬 In discussion
Development

Successfully merging this pull request may close these issues.

Allow changing pinia root store for component scope
3 participants