Web-components: Fix cleaning Lit Expression comments

[muratcorlu]

We are trying to create a design guideline with lit and want to use storybook to document our components. Here is a example story (in mdx format) that I use:

import { html } from 'lit';
import { Canvas, Story } from '@storybook/addon-docs';
import './gr-button';

export const Template = (args) => html`<gr-button 
    ?secondary=${args.variant === 'secondary'} 
    ?disabled=${args.disabled} 
    ?block=${args.block} 
    ?medium=${args.size === 'medium'} 
    ?small=${args.size === 'small'}>${args.label}</gr-button>`;

<Canvas columns={1}>
  <Story name="Secondary Block disabled button"  args={{ disabled: true, variant: 'secondary', block: true, label: 'Fill the form' }}>
    {Template.bind({})}
  </Story>
</Canvas>

And here the result that Storybook shows in "Show code" panel in the UI:

<gr-button secondary="" disabled="" block=""><!--?lit$754556085$-->Fill the form</gr-button>

This fix cleans Lit expression comments from output (<!--?lit$754556085$-->)

What I did

There was a regexp replace for cleaning comment tags, I just updated this regexp by taking the one from lit-html repository.

How to test

There was no test case related with regexp replace part in current code. I wanted to add extra test cases for this but I'm very confused while I was trying to write it. I'll check it again but creating this draft anyway to be able to share what I'm doing for solving this issue.

I also think updating kitchen sink app with an example that has the condition above will be helpful.

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit ea837ca. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

✅ Successfully ran 1 target

• nx run-many --target=prepare --all --parallel=2 --max-parallel=15 -- --optimized

---

Sent with 💌 from NxCloud.

[ndelangen]

Hello @muratcorlu great contribution! Is this still in draft? What else is there still to do?

[muratcorlu]

Hello @ndelangen. Sorry, I was on vacation. I wanted to update unit tests for this case but couldn't manage. Some help would be great. Let me try to push my try to discuss on.

[muratcorlu]

  const story = context?.parameters.docs?.source?.excludeDecorators
    ? (context.originalStoryFn as ArgsStoryFn<WebComponentsFramework>)(context.args, context)
    : storyFn();

Here in the sourceDecorator it runs originalStoryFn of the context with provided args if context?.parameters.docs?.source?.excludeDecorators is truthy. Otherwise it calls given storyFn without arguments. I'm new at storybook but as far as I understand, this should be other way around. I just tried to add a storyFn like (args: any) => html`<div>${args.slot}</div>` but since I don't give excludeDecorators: true this function doesn't get any arguments.

I'm not sure if I understand the meaning of excludeDecorators config properly here. Some help would be great.

[yannbf]

Hey @muratcorlu thank you so much for your contribution!

Some help would be great.

I updated your branch and fixed some TS issues as well as made your test pass. Could you look at it and make sure it will strip the comments correctly?

Here in the sourceDecorator it runs originalStoryFn of the context with provided args if context?.parameters.docs?.source?.excludeDecorators is truthy. Otherwise it calls given storyFn without arguments.

The storyFn is something a bit more complex, which is prepared in Storybook and has combined decorators as well as a bound context. Even if it's called on its own with no arguments, it will still execute correctly, so that code is correct. I tweaked your test to simulate that behavior. If you want to know more about how stories are prepared you can check here!

I'm not sure if I understand the meaning of excludeDecorators config properly here. Some help would be great.

Some might want to present the code snippet to users without decorators, and that's why we provide that option. One example is a React component that uses theming and the theming comes from a decorator:

const withTheme = (storyFn) => {
  return <ThemeProvider>{storyFn()}</ThemeProvider>
}

const myStory = (args) => <Button {...args} />

So you could opt to either get the Button snippet or Button + Provider, by setting the excludeDecorators parameter.

I hope this helps!

[muratcorlu]

Hi @yannbf Thanks for the fixes and detailed explanation. That really helped me to understand what is happening here.

I think currently, this PR fulfills its requirements, but as far as I see, some other tests in the next branch fail, therefore this PR. Other than that, everything looks ok to me.

[yannbf]

Hi @yannbf Thanks for the fixes and detailed explanation. That really helped me to understand what is happening here.
I think currently, this PR fulfills its requirements, but as far as I see, some other tests in the next branch fail, therefore this PR. Other than that, everything looks ok to me.

Actually it's all good. We are experiencing some issues with both e2e-tests-pnp and e2e-tests-sb-docs that are unrelated to your changes. Thank you for your contribution! Were you able to check whether this worked in your project as well? You could do so by replacing the contents of your node_modules/@storybook/web-components/dist with the ones from your local repo like storybookjs/app/web-components/dist

[muratcorlu]

I tried to test in our environment, I also needed to link addon-docs package. I linked both and had a successful build but once I run storybook I see an error like below and empty pages in the browser:

Unhandled Promise Rejection: Error: No `docs.container` set, did you run `addon-docs/preset`?

Does this mean anything to you?

[yannbf]

I tried to test in our environment, I also needed to link addon-docs package. I linked both and had a successful build but once I run storybook I see an error like below and empty pages in the browser:

Unhandled Promise Rejection: Error: No `docs.container` set, did you run `addon-docs/preset`?

Does this mean anything to you?

I'm not sure, I'd have to see your codebase.

If I could guess, my instructions were not clear and the correct way to test this would be:
You could do so by upgrading storybook in your project with npx sb@next upgrade --prerelease then replacing the contents of your node_modules/@storybook/web-components/dist with the ones from your local repo like storybookjs/app/web-components/dist

Because the versions should match. Maybe you moved the contents to the dist folder but your local Storybook version is 6.4? Feel free to reach out if you want to pair on this!

[muratcorlu]

Hi @yannbf. I managed to test this within our project and I can confirm this works properly to clean Lit expression comments. I also tested on sb@next without copying my local changes and saw the issue again. So, I think we are safe to go. 🚀

[yannbf]

Awesome! Thanks for letting me know