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: remove Nuxt context conflict #723

Merged
merged 2 commits into from
Aug 3, 2024

Conversation

obulat
Copy link
Contributor

@obulat obulat commented Jul 16, 2024

πŸ”— Linked issue

Related #661

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This PR updates the global nuxt context, setting its nuxt property to the story's nuxt when setting up the current story.

Previously, when navigating from one story to another story, Nuxt would get the context using the app's _name, which is the same for all stories, instead of using the globalName, which is different for each story.

Copy link

netlify bot commented Jul 16, 2024

πŸ‘· Deploy request for nuxt-storybook pending review.

Visit the deploys page to approve it

Name Link
πŸ”¨ Latest commit 9c3f84b

@obulat obulat force-pushed the fix/nuxt-context-conflict branch from ee127fb to b7634fd Compare July 16, 2024 13:14
@CarlosZoft
Copy link

@tobiasdiez

image

@CarlosZoft
Copy link

@obulat Do you have a temporary solution to this problem?

@obulat
Copy link
Contributor Author

obulat commented Jul 18, 2024

@obulat Do you have a temporary solution to this problem?

No, unfortunately I don't.

@tobiasdiez
Copy link
Collaborator

Awesome! On a first glance it looks good to me, will do a bit more testing in a few days and then merge.

Copy link
Collaborator

@chakAs3 chakAs3 left a comment

Choose a reason for hiding this comment

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

Initially, each story in my code had its own Nuxt instance because each story functioned as a separate Vue app mounted in the Storybook canvas. I used the first Nuxt instance as a shared context with the Nuxt app project configuration. However, with recent changes to the package structure, I'm wondering about how it was working previously without this fix. Additionally, these changes have affected my ability to test it before merging or releasing. @tobiasdiez should we start writing tests ?

@obulat obulat force-pushed the fix/nuxt-context-conflict branch from b7634fd to 0be9f1e Compare July 24, 2024 10:28
@guim4dev
Copy link

kermit

@tobiasdiez
Copy link
Collaborator

Initially, each story in my code had its own Nuxt instance because each story functioned as a separate Vue app mounted in the Storybook canvas.

That's still the case. But nuxt has now initial support for multi-app projects, so the global context didn't seem to be necessary anymore.

@obulat what exactly is the need for the global context now? Do you know if

Nuxt would get the context using the app's _name, which is the same for all stories

is by design or may be a nuxt bug?

@obulat
Copy link
Contributor Author

obulat commented Jul 25, 2024

@obulat what exactly is the need for the global context now? Do you know if

Nuxt would get the context using the app's _name, which is the same for all stories

is by design or may be a nuxt bug?

From my research globalName was introduced in Nuxt 2 and the intention was to have it removed in Nuxt 3. globalName is still present in Nuxt3 code, but I think it's not used for anything.

._name was introduced in a recent PR that, as you say, @tobiasdiez, started enabling multi-app support in Nuxt: feat(nuxt): allows the support of multiple shared runtime context

By default, it is taken from '#build/nuxt.config.mjs':

https://github.com/nuxt/nuxt/blob/8abb1df01783876c3a39a41f24a4f86b12dd81ae/packages/nuxt/src/app/nuxt.ts#L24

Is it possible to set the Nuxt options' appId when setting up the Nuxt app in the preview.ts? Or would it be better to suggest to Nuxt to make the _name configurable in createNuxtApp call:

https://github.com/nuxt/nuxt/blob/8abb1df01783876c3a39a41f24a4f86b12dd81ae/packages/nuxt/src/app/nuxt.ts#L251

@tobiasdiez
Copy link
Collaborator

@obulat Thanks a lot for the investigation. Yesterday, I've created two PRs on the nuxt repo that should implement your findings: nuxt/nuxt#28391 and nuxt/nuxt#28392.

In the meantime we go with your solution, as it is clearly an improvement. I still get context issues in the "docs" section of the storybook, when multiple controls are displayed there. But single stories are working fine.

@tobiasdiez tobiasdiez merged commit f7fe957 into nuxt-modules:main Aug 3, 2024
19 checks passed
@obulat
Copy link
Contributor Author

obulat commented Aug 7, 2024

@obulat Thanks a lot for the investigation. Yesterday, I've created two PRs on the nuxt repo that should implement your findings: nuxt/nuxt#28391 and nuxt/nuxt#28392.

In the meantime we go with your solution, as it is clearly an improvement. I still get context issues in the "docs" section of the storybook, when multiple controls are displayed there. But single stories are working fine.

Thank you for all your work investigating and opening new upstream PRs, @tobiasdiez !

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.

5 participants