-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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: Rollback v7 breaking change and keep reactive v6-compatible API #22229
Conversation
b526f03
to
35cf9bd
Compare
90f7eee
to
b853be5
Compare
i completely agree with you, and moreover they don't understand how Storybook works. we need to work on this to make it clear for newbie and experts. better documentation and code examples. |
code/renderers/vue3/src/render.ts
Outdated
// this seem to cause recursive updates, and vue errors | ||
// if (slotsMap.has(storyID)) { | ||
// const app = slotsMap.get(storyID); | ||
// if (app?.reactiveSlots) updateArgs(app.reactiveSlots, slots); | ||
// return app?.reactiveSlots; | ||
// } | ||
slotsMap.set(storyID, { component, reactiveSlots: slots }); |
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.
@chakAs3 I think I can remove the whole slotsMap
. All the reactive test passes without it.
My gut feeling is that line 25, fixes this.
Without it, the slots are basically static, and need to updated forcefully.
But now we fix the render function to be lazy, we don't need this code anymore.
Hi Kasper I'm quite available most time on Discord answering, and github answering users issues,i'm not getting any feedback apart from the one that you forwarded to me and even these guys are not coming back to us although all this their unique use case, if there is any other channel that you use to receive feedback please share with me, i can't tackle vue issues. |
Here to be honest i don't see any difference in Storybook api, render function return a Renderer Component.
??? again i don't know how you saw it invoked a second time ? const vueApp = createApp({
setup() {
storyContext.args = reactive(storyContext.args);
const rootElement = storyFn(); // call the story function to get the root element with all the decorators
const args = getArgs(rootElement, storyContext); // get args in case they are altered by decorators otherwise use the args from the context
const appState = {
vueApp,
reactiveArgs: reactive(args),
};
map.set(canvasElement, appState);
return () => {
return h(rootElement, appState.reactiveArgs);
};
},
}); |
This comment was marked as outdated.
This comment was marked as outdated.
is this a confident statement? i feel like i don't know Storybook really this is not anymore about Vue, but about How Storybook real works . @shilman @yannbf @tmeasday . Please correct me if i'm wrong. Component Story in its default mission, is to document different state of a specific Component. so basic and simple format is const meta = {
component: Button,
} satisfies Meta<typeof Button;
export const Primary: Story = {
args: {
primary: true,
label: 'Button',
},
}; It's important to note that this is just the basic format of a Component Story, the custom one does not necessarily require the For example, you can define a export const CustomStory: Story = {
args: {
customStoryArg: 'customStoryArg',
},
render: (args: any) = ({
setup() {
return { args };
},
template: '<div I don\'t need the Button in this Story. Hello, some custom args: {{ args.customStoryArg }} </div',
}),
}; In this example, the You can still document your component while also creating custom stories by customizing the render, which allows for greater flexibility and creativity. However, as Kasper points out, there may be downsides to this approach if something other than the component is on the root of the template, such as in the case of Regardless of the template you use to create your custom rendering component - whether it's |
In general, I disagree with the approach of completely wiping out a unit test suite and reverting back to an old implementation (last major version) when we receive a specific issue from a user who might not have a good understanding of how Vue and Storybook work. Instead, I prefer trying to fix the user's issue within the current implementation and teach them how Storybook and Vue really work. This presents a good opportunity to showcase what Storybook can do with Vue and can also attract new developers. Reverting back to a poor implementation is a significant regression that doesn't benefit anyone. So as storybook@vue maintainer i can't approve this PR, anyone from the team @yannbf @shilman can do that if he feels confident. |
…ible' into kasper/vue3-reactivity-v6-compatible # Conflicts: # code/renderers/vue3/src/render.ts
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 so much for this. We didn't realize the new API had been introduced in v7. @chakAs3 and I will write an RFC for why it's important and more Vue-like to automatically pass args as props to stories, and we will seriously consider updating the story API to make it more idiomatic for Vue users.
If you consider the new API as a feature, removing it is a breaking change and should only happen in a major release with proper deprecation, etc.
However, the new API was not documented until Kasper wrote tests for it, and is still not documented--our official docs for Vue3 all recommend using the setup function (the v6 API). So it's more like an accidental change that snuck in. Removing it is still breaking for anybody who uses it, and it sucks. But it sucks less than committing ourselves to an API that we are not yet comfortable with.
I appreciate your passion around the alternative, your patience to discuss this via an RFC, and of course all of the hard work you're doing to make Storybook the best way to develop, document, and test Vue components in isolation.
@shilman @kasperpeulen I got your point maybe we did not do things properly, I just wanna mention this may not have a lot to do with this specific issue but it is a really good opportunity to be more explicit, and document our API before any release, I guess even with Old v6 Vue developers were confused as they don't find much content for vue, it was more about react, we will put more effort, @kasperpeulen, @jonniebigodes, @kylegach, @integrayshaun and myself I will provide necessary info and examples. |
Here is the problem I saw that, and the guy who proposed the implementation was giving this as a solution, he said clearly in His PR I'm not sure if the Vue community will agree with that, unfortunately, that was no one to veille on Vue implementation, and no one has commented this so you went ahead and merged this, and here we had the real breaking change happened for me. My RFC will not be the PR for fallthrough attributes but more exposing and normalizing Storybook API. |
Just to chime in. I'm very glad to see this getting rolled back for now. All of a sudden when I upgraded to v7 my tests were failing because when I applied a 'placeholder' prop to my components and tried to get the element by placeholder text in my test, storybook was finding two elements instead of one. This is because I originally had a decorator div where I would apply spacing and dark tailwind styling. The placeholder prop was being applied to the div and to the component which made no sense and forced me to remove any decorators that I had. |
…compatible Vue3: Rollback v7 breaking change and keep reactive v6-compatible API
…compatible Vue3: Rollback v7 breaking change and keep reactive v6-compatible API
…compatible Vue3: Rollback v7 breaking change and keep reactive v6-compatible API
…compatible Vue3: Rollback v7 breaking change and keep reactive v6-compatible API
…compatible Vue3: Rollback v7 breaking change and keep reactive v6-compatible API
This is a work-in-progress commit. It all started with simply wanting to build our typedoc generated documentation for modules such as chain-api, chaincode, etc. The short summary is that chain-ui is a UI package, designed to contain multiple front-end libraries/packages. It looks like web component and vue versions to start. And the rest of the mono-repo is GalaChain / Hyperledger Fabric based modules that use Node 18 vs 20, and are fully backend. I may or may not return to this in the future. But I'm publishing this just in case it's useful to anyone else who embarks on this build. Maybe a developer that was involved in the initial chain-ui build. Or maybe we will collectively decide to rework our monorepo structure and separate concerns between UI and backend / blockchain / Hyperledger Fabric libraries. These notes are for my future self or others. I was fully unfamiliar with Vue, Vite, tailwind, etc. etc. and the original development context of this entire chain-ui segment of our monorepo prior to attempting to get all this to build together. And I haven't done any front end work since sometime prior to November 1, 2021. It feels like I made it almost all the way through troubleshooting the monorepo build of galachain/sdk, post-chain-ui merge. However, solving each hurdle seemed to lead to another and eventually I had to move on to other engagements. Rough notes follow Some errors in progress of working through: $ npm run typedoc-chain-api ``` > @gala-chain/sdk@1.4.2 typedoc-chain-api > typedoc --tsconfig ./tsconfig.base.json --hideGenerator --plugin typedoc-plugin-markdown --githubPages false --out ./docs/chain-api-docs ./chain-api/src && rm ./docs/chain-api-docs/README.md [info] Loaded plugin typedoc-plugin-markdown chain-ui/src/components/MintToken.stories.ts:18:23 - error TS2307: Cannot find module './MintToken.vue' or its corresponding type declarations. 18 import MintToken from './MintToken.vue' ~~~~~~~~~~~~~~~~~ chain-ui/src/components/MintTokenWithAllowance.stories.ts:18:36 - error TS2307: Cannot find module './MintTokenWithAllowance.vue' or its corresponding type declarations. 18 import MintTokenWithAllowance from './MintTokenWithAllowance.vue' ``` Many, many more errors like the above follow - lots of TypeScript compilation problems. Possibly in part due to the root of the mono-repo using a different tsconfig than the genesis of this chain-ui package, I'll warrant. ``` Rollup failed to resolve import "primevue/inputText" If you do want to externalize this module explicitly add it to`build.rollupOptions.external` ``` ``` npm i --install-strategy=nested tailwindcss-primeui ``` This link looks similar to what we have - https://storybook.js.org/docs/writing-stories/typescript ``` type Story = StoryObj<typeof Button>; ``` storybookjs/storybook#23352 It appears maybe 7.0.18 broke non-prop custom args Vue3 + TypeScript storybookjs/storybook#22229 And maybe this rolled back some breaking changes seems we're using CSF 2, version 3 migration guide is available and is also discussed in improved ts features blog post https://storybook.js.org/docs/api/csf?ref=storybookblog.ghost.io#upgrading-from-csf-2-to-csf-3 https://storybook.js.org/blog/storybook-csf3-is-here/?ref=storybookblog.ghost.io https://storybook.js.org/blog/improved-type-safety-in-storybook-7/ ``` executed in chain-ui folder $ npm install --install-strategy=nested primevue ``` ``` $ grep primevue package-lock.json "primevue": "^3.53.0", "chain-ui/node_modules/primevue": { "resolved": "https://registry.npmjs.org/primevue/-/primevue-3.53.0.tgz", ``` ran in root dir, doesn't solve ``` npm install -w chain-ui --install-strategy=nested primevue cd chain-ui npm install --install-strategy=nested primevue ``` troubleshooting ``` delete primevue from chain-ui/package.json cd .. npm install cd chain-ui npm install --save --install-strategy=nested primevue ``` https://docs.npmjs.com/cli/v10/using-npm/workspaces https://docs.npmjs.com/cli/v10/using-npm/workspaces#defining-workspaces https://docs.npmjs.com/downloading-and-installing-packages-locally https://docs.npmjs.com/cli/v10/commands/npm-install#install-strategy https://docs.npmjs.com/cli/v10/commands/npm-install#configuration https://docs.npmjs.com/cli/v10/configuring-npm/npmrc https://docs.npmjs.com/cli/v10/configuring-npm/package-json?v=true#bundledependencies https://nx.dev/concepts/decisions/why-monorepos https://nx.dev/nx-api/storybook https://nx.dev/nx-api/storybook https://nodejs.org/api/packages.html#conditional-exports https://v3.primevue.org/theming/ https://v3.primevue.org/vite https://stackoverflow.com/questions/61467722/difference-between-npm-update-and-remove-package-lock-json-plus-npm-install https://stackoverflow.com/questions/77755121/vue-tsc-failing-with-error-referenced-project-may-not-disable-emit-on-vue-proj https://stackoverflow.com/questions/76935759/how-to-get-vite-ts-and-vue-to-properly-work-in-vs-code https://www.geeksforgeeks.org/how-to-override-nested-npm-dependency-versions/ https://blog.logrocket.com/comparing-vue-3-options-api-composition-api/ https://stackoverflow.com/questions/71055016/vue-3-defineprops-with-types-and-componentobjectpropsoptions-like-default-or-va https://stackoverflow.com/questions/66288645/vite-does-not-build-tailwind-based-on-config https://vitejs.dev/guide/features.html#postcss https://stackoverflow.com/questions/66288645/vite-does-not-build-tailwind-based-on-config/78451703#78451703 https://tailwindcss.com/docs/content-configuration https://vitejs.dev/config/build-options#build-target https://rollupjs.org/configuration-options/ https://rollupjs.org/configuration-options/#output-hoisttransitiveimports https://rollupjs.org/configuration-options/#context https://nx.dev/recipes/vite/configure-vite https://nx.dev/nx-api/vite https://nx.dev/nx-api/vite/generators/configuration https://stackoverflow.com/questions/70807080/how-to-change-the-ts-path-aliases-for-an-nx-application https://tailwindcss.com/docs/guides/vite#vue https://v2.tailwindcss.com/docs/guides/vue-3-vite https://tailwindcss.com/docs/customizing-colors https://primevue.org/tailwind/ Omitted various links to github issues in related dependencies
In v7 an unplanned/undocumented breaking change was introduced. We apply now args as props to the component returned from the render function:
In this example, that means that the onClick action and the disabled arg are not only applied to
TestBtn
, but also to the outer div. This is because of fall through attributes:https://vuejs.org/guide/components/attrs.html#fallthrough-attributes
This breaking change has as benefit that the user doesn't have to write v-bind="args" here:
template: '<MyComponent v-bind="args" />',
They can leave out
v-bind="args"
, but only if the component is on the root.The major downside of this change is if you have something else than your component on the root:
template: '<div><MyComponent v-bind="args" /></div>',
Now, the args are "magically" spread into a div, but the user didn't write the args for the div, they are written and meant for
MyComponent
. We are getting feedback from multiple Storybook users that people don't expect this, causing unexpected issues.I think the reason that people don't expect this isn't because they don't understand Vue. It's more that they don't expect the template that they give to Storybook to be invoked as a component with their args as props. The user cannot see how SB will invoke the component, and args are already available in the closure. It is not intuitive that args are also available via a second route.
This is different from all our other renderers (or at least the ones I know):
React:
render: (args) => <Component {...args} />
Svelte:
render: (args) => { Component, props: args }
Html:
render: (args) => createButton(args)
Lit:
render: (args) => <lit-element label=${args.label}></lit-element>
Args are just available in the closure and the element returned from render is not invoked a second time.
Also, our TS types and the add-on controls will match the shape of the component, not the root tag of the template. So even our tools don't expect this, and you will get warnings from TS if you would write an arg that is not compatible with the component (even if it would be compatible with the root tag/component).
That being said, @chakAs3 is going to write up a RFC for doing this breaking change behind a feature flag in V7 and if this gains popularity, we can enable this feature by default for v8!