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

Vue: Add Vue 3 support #13775

Merged
merged 10 commits into from
Feb 2, 2021
Merged

Vue: Add Vue 3 support #13775

merged 10 commits into from
Feb 2, 2021

Conversation

phated
Copy link
Contributor

@phated phated commented Jan 30, 2021

Issue: #10654

What I did

This PR adds initial support for Vue 3 through @storybook/vue3 and CLI detection for Vue 3 with sb init.

It is heavily inspired by earlier prototypes by:

All of whom did the initial exploration to get Vue 3 working in storybook!

Notably missing: Vue 3 support in addon-docs and addon-storyshots, which will be added as individual features once this is in alpha. 😄

How to test

  • Is this testable with Jest or Chromatic screenshots? Added an e2e test and generator CLI jest tests
  • Does this need a new example in the kitchen sink apps? Added vue-3-cli example
  • Does this need an update to the documentation? Probably? Any tips?

If your answer is yes to any of these, please make sure to include it in your PR.


---

Storybook also comes with a lot of [addons](https://storybook.js.org/docs/vue3/configure/storybook-addons) and a great API to customize as you wish.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does vue3 section get generated on the docs website?

Copy link
Member

Choose a reason for hiding this comment

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

This needs to be added in the frontpage repo. @jonniebigodes can help with that.

app/vue3/src/client/preview/globals.ts Show resolved Hide resolved

if (innerStory) {
return {
extends: story,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs to be tested. I tried using the extends syntax to support object and function components.

Comment on lines +50 to +52
render() {
return h(story, this.props || this.$props);
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered making this a setup method

forceReRender(): void;
raw: () => any; // todo add type
load: (...args: any[]) => void;
app: App;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct place to put this type?

Comment on lines +12 to +15
// If an end-user calls `unmount` on the app, we need to clear our root variable
unmounted() {
root = null;
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to make sure this works and can be re-mounted.

app/vue3/src/client/preview/render.ts Outdated Show resolved Hide resolved
import { RenderContext, StoryFnVueReturnType } from './types';

const activeStoryComponent = shallowRef<StoryFnVueReturnType | null>(null);
const activeProps = shallowRef<Args>({});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is a shallow ref fine here? I don't think these props need to be wrapped in a reactive, right?


const generator: Generator = async (packageManager, npmOptions, options) => {
baseGenerator(packageManager, npmOptions, options, 'vue3', {
extraPackages: ['vue-loader@^16.0.0'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now e2e testing the version string specifying on extraPackages added in #13774

// TODO(blaine): Remove when we support Vue 3
vue: (versionRange) => versionRange === 'next' || eqMajor(versionRange, 3),
// TODO(blaine): Remove when we support Vue 3
// TODO(blaine): Remove when we support Nuxt 3
nuxt: (versionRange) => eqMajor(versionRange, 3),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I specifically left nuxt here because they don't have v3 out yet and I want to make sure we work with it before allowing it in the generator.

@phated phated added the run e2e extended test suite Run the e2e extended test suite in CircleCI workflows label Jan 30, 2021
@shilman
Copy link
Member

shilman commented Jan 30, 2021

@Aaron-Pool @backbone87 @pocka @graup your careful review much appreciated. @phated pointed out a few questions for vue experts such as yourselves!

@graup
Copy link
Contributor

graup commented Jan 30, 2021

Very nice. I gave the example a spin and it seems to work well so far.
Can't really comment on any of the questions, sorry :( Not really familiar with these parts.
If there's no one else available for review I'm happy to dig deeper but would probably need someone to talk me through it.

@wpitallo
Copy link

Hi, when is the ETA for full Vue3 support?

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

Awesome stuff @phated! I assume the question of whether to make this a separate package or not has been debated already with @shilman?

Comment on lines 12 to 16
const Template = (args, { argTypes }) => ({
props: Object.keys(argTypes),
components: { MyButton },
template: '<my-button v-bind="$props" />',
});
Copy link
Member

Choose a reason for hiding this comment

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

This might not be the right time to ask this, as I understand it works the same way in vue2, but am I right in saying that the package automatically maps context.args (the first argument) to $props in the component?

Is there some way to override this behaviour in the story/template definition? I'm wondering if we could support something like:

const Template = (args, { argTypes }) => ({
    props: [...Object.keys(argTypes), 'extraArg'],
    components: { MyButton },
    template: '<my-button v-bind="$props" />',
    args: {
      ...args,
      extraArg: 'value',
    },
  });

cc @jonniebigodes -- this came up in the development of the vue snippets, IIRC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I honestly have no idea. What happens is that the render function receives args and those are set in shallowRef as the props. Those are then provided to components as props (2nd argument to h(Component, props)) and also use a provide() function to be injected as props too.

Copy link
Member

Choose a reason for hiding this comment

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

@tmeasday something like this in vue3 (untested, based on discussion with @phated) would be the vue way of doing things:

const Template = (args, { argTypes }) => ({
  props: [...Object.keys(argsTypes), 'extraArg'],
  components: { MyButton },
  setup(props) {
    return () => h(MyButton, { ...props, extraArg: 'foo' })
  }
});

We could also potentially bastardize things to make args first class in Vue stories, but this could be confusing to Vue developers since args don't exist in the Vue world. That might look something like this:

const Template = (args) => ({
  components: { MyButton },
  template: '<my-button v-bind="$props" />',
  args: { ...args, extraArg: 'value' },
});

We'd need to post-process this in the Vue framework code to turn it into a valid Vue component.

Copy link
Member

@tmeasday tmeasday Feb 3, 2021

Choose a reason for hiding this comment

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

So the issue here is the "thing" returned by a vue story is a real existing thing in Vue (essentially equivalent to a component in react), so adding an extra args property would be sort of bastardizing it. I get that. (Correct me if I have that wrong)

I think there's sort of a mismatch here between the semantics of a vue and a react story.

// In React
const Template = (args) => <Component {...args} />;

// this means:
const Template = /* a "story component" implemented as a SFC */

// In Vue
const Template = (args) = ({ ... });

// this means
const Template = (args) => /* a "story component" */

Notice the difference? In Vue we are returning a component from the function, which will later be "instanciated" (with the args) via the @storybook/vue framework. That's like fundamentally different, which is why things are a bit hairy.

I suspect (based on not much knowledge) that really the function could be dropped from the vue template. I wonder if that would make things more consistent, and it more natural to do things like modify the args in a setup property.

@shilman shilman added the vue label Feb 1, 2021
@shilman
Copy link
Member

shilman commented Feb 1, 2021

@wpitallo Releasing this in alpha now, hope to get storyshots/docs this week. This will go stable in 6.2, which should be out in March.

@shilman
Copy link
Member

shilman commented Feb 1, 2021

@phated build is failing. This is probably just a version resolution thing, but we'll need to get it sorted out before we can merge.

@storybook/addon-docs: src/frameworks/html/config.tsx(13,25): error TS7006: Parameter 'node' implicitly has an 'any' type.
@storybook/addon-docs: src/frameworks/react/jsxDecorator.tsx(80,24): error TS2786: 'Type' cannot be used as a JSX component.
@storybook/addon-docs:   Its element type 'ReactElement<any, string | ((props: any) => ReactElement<any, string | ... | (new (props: any) => Component<any, any, any>)>) | (new (props: any) => Component<any, any, any>)> | Component<...>' is not a valid JSX element.
@storybook/addon-docs:     Type 'Component<any, any, any>' is not assignable to type 'Element | ElementClass'.
@storybook/addon-docs:       Property '$props' is missing in type 'Component<any, any, any>' but required in type 'ElementClass'.
@storybook/addon-docs: src/frameworks/vue/prepareForInline.ts(18,22): error TS2351: This expression is not constructable.
@storybook/addon-docs:   Type 'typeof import("/tmp/storybook/node_modules/vue/dist/vue")' has no construct signatures.
@storybook/addon-docs: src/frameworks/vue/prepareForInline.ts(26,14): error TS7006: Parameter 'h' implicitly has an 'any' type.
@storybook/addon-docs: src/frameworks/vue/sourceDecorator.ts(39,20): error TS2351: This expression is not constructable.
@storybook/addon-docs:   Type 'typeof import("/tmp/storybook/node_modules/vue/dist/vue")' has no construct signatures.
@storybook/addon-docs: src/frameworks/vue/sourceDecorator.ts(45,14): error TS7006: Parameter 'h' implicitly has an 'any' type.
@storybook/addon-docs: src/frameworks/vue/sourceDecorator.ts(78,15): error TS2339: Property 'data' does not exist on type 'VNode<RendererNode, RendererElement, { [key: string]: any; }>'.
@storybook/addon-docs: src/frameworks/vue/sourceDecorator.ts(78,45): error TS2339: Property 'data' does not exist on type 'VNode<RendererNode, RendererElement, { [key: string]: any; }>'.
@storybook/addon-docs: src/frameworks/vue/sourceDecorator.ts(80,15): error TS2339: Property 'componentOptions' does not exist on type 'VNode<RendererNode, RendererElement, { [key: string]: any; }>'.
@storybook/addon-docs: src/frameworks/vue/sourceDecorator.ts(80,66): error TS2339: Property 'componentOptions' does not exist on type 'VNode<RendererNode, RendererElement, { [key: string]: any; }>'.
@storybook/addon-docs: src/frameworks/vue/sourceDecorator.ts(81,15): error TS2339: Property 'data' does not exist on type 'VNode<RendererNode, RendererElement, { [key: string]: any; }>'.
@storybook/addon-docs: src/frameworks/vue/sourceDecorator.ts(81,50): error TS2339: Property 'data' does not exist on type 'VNode<RendererNode, RendererElement, { [key: string]: any; }>'.
@storybook/addon-docs: src/frameworks/vue/sourceDecorator.ts(88,14): error TS2339: Property 'componentOptions' does not exist on type 'VNode<RendererNode, RendererElement, { [key: string]: any; }>'.
@storybook/addon-docs: src/frameworks/vue/sourceDecorator.ts(90,15): error TS2339: Property 'tag' does not exist on type 'VNode<RendererNode, RendererElement, { [key: string]: any; }>'.
@storybook/addon-docs: src/frameworks/vue/sourceDecorator.ts(92,26): error TS2339: Property 'tag' does not exist on type 'VNode<RendererNode, RendererElement, { [key: string]: any; }>'.
@storybook/addon-docs: src/frameworks/vue/sourceDecorator.ts(95,24): error TS2339: Property 'tag' does not exist on type 'VNode<RendererNode, RendererElement, { [key: string]: any; }>'.
@storybook/addon-docs: src/frameworks/vue/sourceDecorator.ts(95,60): error TS2339: Property 'map' does not exist on type 'VNodeNormalizedChildren'.
@storybook/addon-docs:   Property 'map' does not exist on type 'string'.

@shilman
Copy link
Member

shilman commented Feb 1, 2021

@tmeasday regarding two packages or one, it's a tradeoff between user/documentation overhead and implementation overhead. In this case, I think with the changes (code, types, etc.) and dependency injection (?) or backflips that would be required to support each variant, I figured it would be better to keep it simple and implement it as a separate package. Now that @phated 's been through the code I'd like to hear what he thinks.

@shilman
Copy link
Member

shilman commented Feb 1, 2021

Also, probably out of scope by WDYT about #8673 but for Vue @phated?

@phated
Copy link
Contributor Author

phated commented Feb 1, 2021

Also, probably out of scope by WDYT about #8673 but for Vue

I don't understand the issue in relation to Vue. The thing returned is the component and you don't need to specify some subprop that defines it.

@phated
Copy link
Contributor Author

phated commented Feb 1, 2021

regarding two packages or one, it's a tradeoff between user/documentation overhead and implementation overhead. In this case, I think with the changes (code, types, etc.) and dependency injection (?) or backflips that would be required to support each variant, I figured it would be better to keep it simple and implement it as a separate package. Now that @phated 's been through the code I'd like to hear what he thinks.

I agree, I don't see how it'd be possible to support them side-by-side.

@phated
Copy link
Contributor Author

phated commented Feb 1, 2021

@shilman those typescript errors aren't happening in vue3, they are happening in other modules. I'm not sure what you want me to do here because yarn bootstrap --core runs perfectly fine locally

Edit: I figured out how to make this happen locally: I had to rm -rf the examples directory and re-install, re-bootstrap everything from a clean state. It looks like yarn then prefers vue 3 at the top level as vue was never specified as a devDep of addon-docs (only a peerDep)

@litan1106
Copy link

@phated Good work! I am excited for this PR release into alpha.

@shilman shilman added this to the 6.2 core milestone Feb 2, 2021
@shilman shilman changed the title Vue 3 Vue: Add Vue 3 support Feb 2, 2021
@shilman shilman merged commit c1de529 into storybookjs:next Feb 2, 2021
@shilman shilman mentioned this pull request Feb 2, 2021
@phated phated deleted the phated/vue3 branch February 2, 2021 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request run e2e extended test suite Run the e2e extended test suite in CircleCI workflows vue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants