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(nuxt): Support nuxt layers in a better way #2699

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

Conversation

marr
Copy link

@marr marr commented Jun 20, 2024

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:

  1. The pinia module tries to resolve pinia/dist/pinia.mjs from the containing app. This is problematic, because the layer is the one bringing the pinia dependency in. It should be using the distributable that the layer depends on.
  2. The stores directory in the layer gets lost when the containing app tries to register its own stores.
  3. Extending the layer should bring in the stores from the layer, and allow adding stores from the containing app.

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.

Copy link

netlify bot commented Jun 20, 2024

Deploy Preview for pinia-playground canceled.

Name Link
🔨 Latest commit 206c1b8
🔍 Latest deploy log https://app.netlify.com/sites/pinia-playground/deploys/66a7c1d2c8b19300085bb0ac

Copy link

netlify bot commented Jun 20, 2024

Deploy Preview for pinia-official canceled.

Name Link
🔨 Latest commit 206c1b8
🔍 Latest deploy log https://app.netlify.com/sites/pinia-official/deploys/66a7c1d296f3010008267873

@marr marr force-pushed the v2 branch 2 times, most recently from 6d67cf0 to f4ec13b Compare June 24, 2024 12:22
Copy link
Member

@posva posva left a 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

for (const storeDir of [
...options.storesDirs,
/* @ts-expect-error storesDirs isn't on base NuxtOptions type */
...(nuxt.options.pinia?.storesDirs || []),
Copy link
Member

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

Copy link
Author

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.

Copy link
Member

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?

Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🧑‍💻 In progress
Development

Successfully merging this pull request may close these issues.

None yet

2 participants