Chip through some situations to get storybook working

[Westbrook]

I'll comment what's going on here inline.

[Westbrook]

This is a collision of v5 vs v6 CFS syntax. @open-wc/demoing-storybook only supports v5 at this time. There is research going into https://github.com/modernweb-dev/storybook-prebuilt where we might move to that in the future, but it's not currently "stable" for release just yet.

[dgp1130]

What do you mean by "collision" here? How did the syntax change between the two versions? Am I running into an ambiguity or something here?

[Westbrook]

On the left you have the syntax from Storybook v6:

const Template = (args: ShareArgs) => html`
    <dwac-share .target="${args.target ? new URL(args.target) : undefined}"
            .articleTitle="${args.articleTitle}"
            .prompt="${args.prompt}">
    </dwac-share>
`;

export const Primary = Template.bind({});
(Primary as any).args = {
    target: 'https://blog.dwac.dev/',
    articleTitle: 'Devel without a Cause',
    prompt: 'Check out my blog!',
};

and on the right you have the v5 syntax:

export const primary = ({
    target = 'https://blog.dwac.dev/',
    articleTitle = 'Devel without a Cause',
    prompt = 'Check out my blog!',
}: ShareArgs = {}) => {
   return html`
        <dwac-share .target="${target ? new URL(target) : undefined}"
                article-title="${articleTitle}"
                prompt="${prompt}">
        </dwac-share>   
    `
;

The .args = {} and .bind({}) syntaxes were breaking your stories in that @open-wc/demoing-storybook doesn't use Storybook v6 yet.

[dgp1130]

Gotcha, so I want to use a function with default values for now rather setting an arguments object. Storybook must be doing some real magic to display these default values then, I can see why they wanted to move to explicitly exported args.

[Westbrook]

These two changes are big part of why you knobs weren't working. Knobs can't represent an undefined value, so nothing that defaults to undefined can be managed by a knob. 😬

[dgp1130]

Interesting, that's probably what my core issue was, as I never saw the type or default value requirement mentioned in any docs and never tried anything like this. Although it seems quite limiting and unfortunate. Is there no way to express an optional property in Storybook? I'd rather not use an empty string here if I can avoid it.

It especially sucks because there is no good way to express required properties in custom elements/Lit Element from my understanding. I just have the // Required. comment here to say that I expect this to be set before render() is called. Combining this with Storybook, I also can't leave it undefined, so instead I need to give it a dummy value.

It feels like Lit Element wants everything to be optional, but Storybook wants everything to be required or optional with a default value. Keeping both of these systems happy seems like kind of a pain.

[Westbrook]

Honestly, I had no idea this was a requirement until I started debugging your repo. Even in places where I've used similar, I've had enough other properties that I hadn't noticed. This is one reason that v6 moved to args/controls as opposed to knobs, IIRC, but we don't support that just yet.

It's not that the disagree on requirements, it's just not clear to Storybook what to do when representing undefined, so, if you don't do it manually, it doesn't do anything. It's all a question of functionality, IMO. Throwing is a hard response to not having a property. I personally return empty/alternate templates or block shouldRender, etc. but for a DOM element to throw is pretty forceful. In that this is a purely local (each element can choose differently based on goals) problem, I'm sure it's not been thought of much at the LitElement level.

[dgp1130]

I'll admit my philosophy on component design is a little unique. I have never been a fan of classes where all the fields are optional. I always ask "If it is valid for every field to be undefined or null, then what does it mean you have an instance with no data in it?" Usually there are fields which are required for the class to function, in this case target is required in order to do anything at all (I currently have articleTitle required, but I supposed I could default to generic text if I really wanted to). After all, what would <dwac-share /> do if it had no URL to share? Usually I handle this with required values in a constructor, but custom elements just don't work that way, so I'm basically forced into a situation where all fields are optional.

The closest answer I have is to assert that the values are defined, treating missing required data as effectively an invalid state which is just detected at runtime rather than compile-time. This at least surfaces the issue to a developer misusing the component (rather than rendering nothing), and more strictly types the fields to defined values thus reducing the valid state space and the number of cases I need to support. I haven't seen any best practices around required properties for Lit Element or custom elements in general, so this has generally been the model I've worked with.

Looking around a little more, it seems like this was fixed in Storybook 5.3.0. I'm not sure exactly what I have installed, but npm ls tells me I have storybook-prebuilt@1.5.2, which I think maps to 5.3.1. However, using an optional articleTitle with a default value provided in the story does not display a knob in Storybook. I also tried using null instead of undefined, but that doesn't work either. I'm wondering if this is a regression in Storybook or if the prebuilt package isn't working the way I think it is.

[Westbrook]

Not sure how you intend to build these tools out, but instead of trying to think of a way to "build" you code, I just skipped all the code that would have needed to be built, instead.

[dgp1130]

Thanks so much for doing this! I really appreciate your time here!

[dgp1130]

Interesting, that's probably what my core issue was, as I never saw the type or default value requirement mentioned in any docs and never tried anything like this. Although it seems quite limiting and unfortunate. Is there no way to express an optional property in Storybook? I'd rather not use an empty string here if I can avoid it.

It especially sucks because there is no good way to express required properties in custom elements/Lit Element from my understanding. I just have the // Required. comment here to say that I expect this to be set before render() is called. Combining this with Storybook, I also can't leave it undefined, so instead I need to give it a dummy value.

It feels like Lit Element wants everything to be optional, but Storybook wants everything to be required or optional with a default value. Keeping both of these systems happy seems like kind of a pain.

[dgp1130]

Is there no way to work with complex types like URL? I'm assuming Storybook doesn't support this out of the box, but it can be trivially constructed from a string, and I even have an attribute converter. If Storybook just used a string knob which assigned this attribute, it would probably work. Is there any way to do that?

[Westbrook]

There are ways, I think. But, they involved deeper hacking on your code than I wanted to risk. The question should be "can the target?: URL; be changed so that the value isn't undefined by default?'. And, if the answer is yes, do that and it should show in the knobs.

[dgp1130]

I tried giving the URL a default value of new URL('http://localhost/'), however that still doesn't show in the knobs. I imagine this could implemented with some special add on that defines a new type of knob which knows how to convert to/from a URL, which would definitely be more effort than it is worth here. But if I already have an attribute converter, I would expect that withWebComponentsKnob could generate text knobs that just assign to the web component attributes and work as expected. Is that not possible/supported, or is there something else I'm missing here?

[dgp1130]

What do you mean by "collision" here? How did the syntax change between the two versions? Am I running into an ambiguity or something here?

[dgp1130]

Is it still necessary to explicitly declare these properties? I thought the point of withWebComponentsKnob was to avoid having to explicitly specify these? Playing with it myself, it seems to work without declaring articleTitle or prompt (target seems to be required because it is a URL type).

[Westbrook]

I do this because you code has the assertDefined calls in it. With the properties set to '' by default, then it's likely that you can get away without this. It would become a matter of how you'd like the stories to be displayed by default and whether you manage that in the story (here) or in the element (the default values of the properties).

[dgp1130]

I see, so its not that they need to be declared here, its that they need a default value to be compatible with Storybook. I guess it makes sense that a component may have some required properties and it would be up to the story to provide a default value.

Ideally, I would love this to be a documentable feature which could be displayed in Storybook. Some kind of @required JSDoc tag which flags the knob in Storybook and displays in the docs. I guess I'm weird in my philosophy on required properties in web components. I can also see some value in specifying a default in the story just so it populates some valid data for a user. Otherwise a required knob might be confusing to a user who doesn't understand what to put in it.

[dgp1130]

So I think the previous commit might have been generated from the Storybook CLI, as it was later that I generated from npm init @open-wc, which did generate this run() command for me.

While I was debugging the knobs, I found that setCustomElements() was being called too late. It appears to set window.__STORYBOOK_CUSTOM_ELEMENTS__ which gets read by withWebComponentsKnobs. However the knob decorator was getting called before setCustomElements() on an initial page load, meaning window.__STORYBOOK_CUSTOM_ELEMENTS__ wasn' set, and withWebComponentsKnobs would do nothing. However, if I clicked to the "docs" tab and back, then window.__STORYBOOK_CUSTOM_ELEMENTS__ was already set and would be read correctly and display knobs in the window.

In fact, just now I'm noticing that running npm run storybook, it will open a new tab and display the knobs as desired. However, if I simply refresh that page, the knobs disappear and I see the message "No knobs found". It only seems to work on first load. However, if I refresh and then click to another story, the knobs will display. This feels like a timing bug in Storybook or the decorator maybe?

Previously I had set open: false in the config because I found it to be really annoying to continuously open new tabs. That means that I was manually refreshing the one tab I had open to Storybook, so that might have added to my confusion as to why the knobs weren't working.

[Westbrook]

We have an open issue for this: open-wc/open-wc#1837 Not sure whether we'd get to it in the current version or if the next version will also exhibit this issue or not. Definitely a timing issue, but not sure 100% where yet. As you said, Storybook is not exactly the most debuggable piece of software.

[dgp1130]

Ah gotcha. I didn't realize this for a while as I only had one story so I had no other story to click to in order to load the knobs. Eventually I found that clicking "Docs" fixes it as well, which is when I started tracing through it. Thanks for linking to that issue, definitely seems to be what I was experiencing.

[Westbrook]

From here, I think much of the "outstanding" ideas are a bit specific to your code. Being in a situation where automatic tooling can be leveraged is a much a context of what your code needs to look like as what you want your code to look like. Hopefully, with the patterns being a little more clear now you'll be able to make decisions that make sense for your project but also play well with Storybook when desired.

I've moved the main sticking points of this conversation into https://open-wc.org/docs/demoing/storybook/ so that hopefully the next person gets caught on new issues that we can be sure to include!

[Westbrook]

We have an open issue for this: open-wc/open-wc#1837 Not sure whether we'd get to it in the current version or if the next version will also exhibit this issue or not. Definitely a timing issue, but not sure 100% where yet. As you said, Storybook is not exactly the most debuggable piece of software.

[Westbrook]

I do this because you code has the assertDefined calls in it. With the properties set to '' by default, then it's likely that you can get away without this. It would become a matter of how you'd like the stories to be displayed by default and whether you manage that in the story (here) or in the element (the default values of the properties).

[Westbrook]

On the left you have the syntax from Storybook v6:

const Template = (args: ShareArgs) => html`
    <dwac-share .target="${args.target ? new URL(args.target) : undefined}"
            .articleTitle="${args.articleTitle}"
            .prompt="${args.prompt}">
    </dwac-share>
`;

export const Primary = Template.bind({});
(Primary as any).args = {
    target: 'https://blog.dwac.dev/',
    articleTitle: 'Devel without a Cause',
    prompt: 'Check out my blog!',
};

and on the right you have the v5 syntax:

export const primary = ({
    target = 'https://blog.dwac.dev/',
    articleTitle = 'Devel without a Cause',
    prompt = 'Check out my blog!',
}: ShareArgs = {}) => {
   return html`
        <dwac-share .target="${target ? new URL(target) : undefined}"
                article-title="${articleTitle}"
                prompt="${prompt}">
        </dwac-share>   
    `
;

The .args = {} and .bind({}) syntaxes were breaking your stories in that @open-wc/demoing-storybook doesn't use Storybook v6 yet.

[Westbrook]

There are ways, I think. But, they involved deeper hacking on your code than I wanted to risk. The question should be "can the target?: URL; be changed so that the value isn't undefined by default?'. And, if the answer is yes, do that and it should show in the knobs.

[Westbrook]

Honestly, I had no idea this was a requirement until I started debugging your repo. Even in places where I've used similar, I've had enough other properties that I hadn't noticed. This is one reason that v6 moved to args/controls as opposed to knobs, IIRC, but we don't support that just yet.

It's not that the disagree on requirements, it's just not clear to Storybook what to do when representing undefined, so, if you don't do it manually, it doesn't do anything. It's all a question of functionality, IMO. Throwing is a hard response to not having a property. I personally return empty/alternate templates or block shouldRender, etc. but for a DOM element to throw is pretty forceful. In that this is a purely local (each element can choose differently based on goals) problem, I'm sure it's not been thought of much at the LitElement level.