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: Support multiple setup functions #22170

Merged
merged 22 commits into from
Apr 25, 2023
Merged

Conversation

chakAs3
Copy link
Contributor

@chakAs3 chakAs3 commented Apr 19, 2023

Closes #

vue 3 single use of setup() which is causing issue for add-on authors NickMcBurney/storybook-vue3-router#38

What I did

change the implementation of setup function mostly called in preview.js or plugins , now you can use as many as you want and it won't be overridden by any plugin use

this PR should be merged after merging #21956

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 shilman April 19, 2023 17:00
@chakAs3 chakAs3 self-assigned this Apr 19, 2023
Copy link
Contributor Author

@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.

i don't like !, or ts-ignore but sometime i can't find solution so that it will be my last solution to fix type check

code/renderers/vue3/template/stories/GlobalSetup.vue Outdated Show resolved Hide resolved
Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

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

these changes look great, a few minor suggestions.

code/renderers/vue3/src/render.ts Show resolved Hide resolved
@chakAs3 chakAs3 requested a review from JReinhold April 20, 2023 21:09
Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

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

Great! added a few minor comments, and there's still one question from previously you haven't resolved yet.

@chakAs3 chakAs3 requested a review from JReinhold April 22, 2023 12:08
@chakAs3
Copy link
Contributor Author

chakAs3 commented Apr 24, 2023

@JReinhold is all green now.

Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

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

Changes look good from me, but I think @kasperpeulen should have a chance of reviewing it before it's merged.

You also haven't answered my comment here, you've just resolved it: https://github.com/storybookjs/storybook/pull/22170/files#r1173060551
can you tell me why my assumption is maybe wrong?

also i don't see why Kasper should review this as he does not have any context of this feature he was not in the Big Thread in Discord i guess @shilman is the one able to review this, we had a big discussion about this with other developers. it is tested and working fine for them it is been 3 weeks waiting for that. i don't know why we really move slowly for such trivial changes ?

@chakAs3
Copy link
Contributor Author

chakAs3 commented Apr 25, 2023

Changes look good from me, but I think @kasperpeulen should have a chance of reviewing it before it's merged.

You also haven't answered my comment here, you've just resolved it: https://github.com/storybookjs/storybook/pull/22170/files#r1173060551 can you tell me why my assumption is maybe wrong?

i already answered you long back @JReinhold the answer just after your comment

image

@chakAs3
Copy link
Contributor Author

chakAs3 commented Apr 25, 2023

@blowsie you still waiting for this to be merged ? when did you tested with your team and when we had the initial discussion about this issue that breaks the work for add-on's authors. means you really can't even have 2 add-ons that utilise app.use(plugin) or app.provide(). works properly, i just need to give @JReinhold more info and context of the issue since he was not in the loop

@blowsie
Copy link

blowsie commented Apr 25, 2023

Yes we are still waiting for this to be merged.
@JReinhold some more context can be found here.
https://discord.com/channels/486522875931656193/1090996147620950067

@blowsie
Copy link

blowsie commented Apr 25, 2023

In short, the setup function provided by storybook, can only be used once, since any further use overrides the last use.
https://github.com/storybookjs/storybook/blob/next/code/renderers/vue3/src/render.ts#L29

Copy link
Contributor Author

@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.

@blowsie do have any suggestion ? you can added your review if it seems fine for you then approve

@blowsie
Copy link

blowsie commented Apr 25, 2023

@blowsie do have any suggestion ? you can added your review if it seems fine for you then approve

Sorry any suggestion for the code changes? Or is there an issue that needs addressing?

@JReinhold
Copy link
Contributor

@chakAs3 you'll notice that the comment you screenshotted comment has a "Pending" tag next to your name, which means that it was never published and is only visible by you. You probably wrote it as part of a review that you never published.

Either way, thanks for the elaboration. I didn't know that the setup() function was only meant to be invoked globally in preview.js, then it makes sense.

@chakAs3
Copy link
Contributor Author

chakAs3 commented Apr 25, 2023

@chakAs3 you'll notice that the comment you screenshotted comment has a "Pending" tag next to your name, which means that it was never published and is only visible by you. You probably wrote it as part of a review that you never published.

Either way, thanks for the elaboration. I didn't know that the setup() function was only meant to be invoked globally in preview.js, then it makes sense.

sorry i found out that was pending late i thought i published already

@chakAs3 chakAs3 requested a review from JReinhold April 25, 2023 12:09
@chakAs3
Copy link
Contributor Author

chakAs3 commented Apr 25, 2023

@JReinhold can approve the requested changes please ! we are stuck here !!

@shilman shilman changed the title Vue3: setup functions multi use implementation Vue3: Support multiple setup functions Apr 25, 2023
Copy link
Contributor

@kasperpeulen kasperpeulen left a comment

Choose a reason for hiding this comment

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

Looks good to me, and thanks!

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

This looks good to me. I'd like @kasperpeulen to make sure this is well-tested, but we can do that after the fact since users are blocked on this. Thanks so much for putting it together. Great change!

@shilman
Copy link
Member

shilman commented Apr 25, 2023

this PR should be merged after merging #21956

@chakAs3 Is this still true? If so, we need to get that one reviewed & merged too?

@chakAs3
Copy link
Contributor Author

chakAs3 commented Apr 25, 2023

this PR should be merged after merging #21956

@chakAs3 Is this still true? If so, we need to get that one reviewed & merged too?

it is already done the PR is merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants