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

Vue3: Fix Renderer Disfunction #22994

Closed
wants to merge 15 commits into from
Closed

Vue3: Fix Renderer Disfunction #22994

wants to merge 15 commits into from

Conversation

chakAs3
Copy link
Contributor

@chakAs3 chakAs3 commented Jun 9, 2023

Closes #

What I did

Recently we got many weird issues, related to vue3 renderer behavior, despite no reactive slots, and no support for CSF2, no support for some Vue main features, ...... these were quite ignored since it concerns only Vue3.
However, this starts to impact the whole storybook system.

  • we are forced to spread context.args, which is not natural.
  • no reactive decorators ( was fixed but has a potential based on my review)
  • weird behavior of conditional decorators.

I just reverted to my initial implementation and everything works just fine
( there is no breaking change

How to test

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@chakAs3 chakAs3 requested a review from a team as a code owner June 9, 2023 06:27
@chakAs3 chakAs3 changed the title Vue3:fix renderer misfunction Vue3: fix renderer disfunction Jun 9, 2023
@chakAs3 chakAs3 changed the title Vue3: fix renderer disfunction Vue3: Fix renderer disfunction Jun 9, 2023
@chakAs3 chakAs3 changed the title Vue3: Fix renderer disfunction Vue3: Fix Renderer Disfunction Jun 9, 2023
@chakAs3 chakAs3 added the ci:daily Run the CI jobs that normally run in the daily job. label Jun 10, 2023
@shilman shilman self-assigned this Jun 10, 2023
@shilman
Copy link
Member

shilman commented Jun 10, 2023

@chakAs3 thanks for being on top of all the reactivity issues in Vue, and SB issues in general. I really appreciate the hard work you're doing here.

However, when it comes to this PR, we've discussed adding Vue2-style story reactivity to Vue3 countless times--on Discord, issues/PRs, and in video calls--and we're not going to do it this way.

Why is this a non-starter?

@kasperpeulen consulted you, the core team, and Vue experts before rolling back your first attempt to add Vue2-style story reactivity. There are multiple comments on the bottom of that PR and in the related issues thanking us for that change.

After this, we are firmly convinced that this is not a suitable change for 7.x. We are open to longer term changes as I explain below.

How do we move forward in the short term?

➡️ In the short term, let's be precise about the "disfunction" in the current system. By precise I mean:

  • Add a test case to demonstrate a failure
  • Create an incremental change to fix the broken test case

We'll review and merge these kinds of PRs. Larger PRs that try to sneak in multiple fixes at once and don't contain test cases are difficult to review and reason about.

How do we move forward for the long term?

We all agree that Storybook's reactivity model and API can be improved and we've collaboratively co-authored an early RFC for how to improve it.

There are multiple questions on that doc. Some of them are Vue-specific and reference you. Many of them relate to other renderers like Svelte and Angular.

➡️ To help this move forward in the long term, you can respond in the comments and help get these questions answered. If you DM me your email, I'm also happy to give you edit access to the doc if that helps.

➡️ Once we agree on an API that is suitable for all the renderers, we can prototype it in Vue behind a feature flag and start using it.

Let's do this 💪

SB for Vue usage has almost doubled over the past year, which is amazing for a mature project like Storybook. I assume this growth is partially due to (1) first-class Vite support in SB7, and (2) the tremendous Vue improvements you've been driving with @kasperpeulen.

Let's collaborate better and figure out how to take Vue support to the next level. In my opinion, this PR is a step backwards.

@chakAs3
Copy link
Contributor Author

chakAs3 commented Jun 10, 2023

Hi @shilman, thank you for sharing your thoughts. I completely agree with your vision and the points you've mentioned. Specifically, I have been working on providing a solution and explanation for the unexpected behavior. @tmeasday and Kasper have expressed this through in 'conditional decorators' PR. The issue we're facing is that we encounter unpredictable and unfixable problems. As a result, we have been relying on workarounds that may introduce additional issues due to the unstable foundation. Although the decision to revert back has been made, I believe it doesn't mean we should neglect fixing this specific part. You could consult with me regarding the decorator issue because I have a clear understanding of the problem. Together, we may be able to come up with a solution, even with the current implementation.

I share your excitement for what lies ahead for Storybook. It represents a significant step, and I believe there is a lot in store to take it to the next level. Therefore, I am eager to enhance collaboration and communication within the team. 🤟 ❤️

@chakAs3
Copy link
Contributor Author

chakAs3 commented Jun 12, 2023

I close in favor of #23029 more narrow one

@chakAs3 chakAs3 closed this Jun 12, 2023
@stof stof deleted the chaks/vue3-impl branch August 2, 2023 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ci:daily Run the CI jobs that normally run in the daily job. vue3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants