-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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(nuxt): Support nuxt layers in a better way #2699
base: v2
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for pinia-playground ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for pinia-official canceled.
|
6d67cf0
to
f4ec13b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
Adding the test would be a nice next step.
Regarding Nuxt 2, it should still work with nuxt bridge
packages/nuxt/src/module.ts
Outdated
for (const storeDir of [ | ||
...options.storesDirs, | ||
/* @ts-expect-error storesDirs isn't on base NuxtOptions type */ | ||
...(nuxt.options.pinia?.storesDirs || []), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was added so that apps that extend layers using this module can still provide storesDirs
that would be auto-imported. For example, if myapp had stores in a mystores
directory, it could add those, while still importing stores the layer defines in its own context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the layer adds options, I suppose they should appear in the options
object. If not, it might be an intentional design for layers in Nuxt. Or are you referring to something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thank you for correcting. I added tests and used explicit storesDirs
in the layer config now to ensure that its stores get added.
6b176e0
to
206c1b8
Compare
@posva is this still something you might consider? |
Yes! I will check it in my next pass |
Thank you! If you are trying to reproduce the error you can follow these steps. Its also reproducible using a layer but you can also do it using a module like so:
Also note that this doesn't seem to happen using pnpm, so make sure to use npm for the reproduction. |
The line that was problematic for layers has been removed. Could you confirm there is no need for something now? |
commit: |
When using the pinia nuxt module with nuxt layers, there were some things that don't appear to work correctly.
I am trying to solve for a nuxt layer workflow, which adds the pinia module and registers some stores in the base layer. I want those stores to be available to my containing app, as well as stores that the containing app defines.
The issues I found with the current state of the module include:
I believe this PR solves those three problems. We remove some use of a deprecated API as well.
To add tests to this, I think I'd need to create a new "playground-layer" directory and extend from that in a new nuxt spec pertaining to layer functionality. If you agree with these changes, we can discuss if you'd like that as part of this.
Note: This may break nuxt 2 compatibility. I can try to add back support if this is a decent enough approach to fixing layer support.