Web-components: Add typescript types and CLI template #12395

tooppaaa · 2020-09-07T06:29:38Z

Issue: #12026 #11974 #11845

What I did

Review awesome work from @43081j
Setup js / ts structure for web components + e2e
web-components typings

How to test

E2E

43081j

Left a couple of comments but looks good to me otherwise. Thanks for picking it up

43081j · 2020-09-07T17:46:59Z

lib/cli/src/frameworks/web-components/ts/Button.ts

+    <button
+      type="button"
+      class=${['storybook-button', `storybook-button--${size || 'medium'}`, mode].join(' ')}
+      style=${backgroundColor && { backgroundColor }}


To be aligned with lit best practice we should use ifDefined(backgroundColor) here. It can be imported from lit-html/directives/if-defined

I'm not a web component expert but if I'm right, there's other options than lit-html, we tried to limit this dependency in the code we provide.
@shilman WDYT ?

Yeah I get that. I wasn't sure either, we want it to be as generic as possible so it isn't tied into one library.

I've since realised too I wasn't quite right as in lit we wouldn't even set this property. We could just leave it as an example if you want to keep it generic though

43081j · 2020-09-07T17:47:43Z

app/web-components/src/client/preview/types.ts

+ *
+ * @see [Default export](https://storybook.js.org/docs/formats/component-story-format/#default-export)
+ */
+export type Meta<Args = DefaultArgs> = BaseMeta<string> & Annotations<Args, StoryFnHtmlReturnType>;


Now that these are in a "types-6" file, should they still live here a second time? If so can you explain why for future reference

My bad, they should not be here. good catch

43081j · 2020-09-07T17:50:38Z

lib/cli/src/frameworks/web-components/ts/Button.ts

@@ -0,0 +1,33 @@
+import { html } from 'lit-html';
+
+export interface ButtonProps {


I wonder if we should make all properties here optional or ButtonProps should be exported as a partial type (type ButtonProps = Partial<{...}>). To simplify all the references to just ButtonProps

Partial means you can skip mandatory properties. Which is not what we want

It does but my point is everywhere we reference this, we use Partial anyway. So we should probably solve the problem at the interface here rather than every time it gets used (which could mean using partial here or just making the properties optional).

We have the same interface for all frameworks. Do you have an example of where Partial would bring value ?

I'm not arguing FOR partial im telling you its used already in this PR:

const Template: Story<Partial<ButtonProps>> = (args) => Button(args);

in the stories.

which in every other framework's stories would instead be:

const Template: Story<ButtonProps> = (args) => Button(args);

so i was just trying to point out that maybe instead of using partial, we should make the optional properties in ButtonProps optional. If that's all of them, fine, or consider using partial when defining the type rather than when using it.

tooppaaa · 2020-09-07T20:10:59Z

lib/cli/src/frameworks/web-components/js/Page.js

@@ -1,6 +1,5 @@
 import { html } from 'lit-html';
 import { Header } from './Header';
-import './page.css';


@43081j I'f I'm right, you removed it, is it automatically importing css ?

I removed it originally because when i changed to typescript, the import couldn't be found. Where does this CSS live?

Really I don't think it should exist for the web components examples anyway.

Each component has its own scoped styles (if you decide to give it any) so it wouldn't make any sense trying to import a css file here. As you would be applying css to the document which can't even penetrate the shadow dom boundaries of these components. So I don't think it's a use case anymore unlike react for example

In real world you would import an object like:

import {styles} from './pageStyles.js';

Which is (in lit):

export const styles = css` :host { display: block; } `;

In use:

class Foo extends LitElement { static styles = styles; }

Styles are always scoped as per the spec so most libraries will do it this way. Or they will produce something like this at build time

Styles are shared between all frameworks and are added on user side. What you propose is probably how it should be done but we also need to find the balance with the current architecture. using css files is the best option

The problem is, using CSS files just wouldn't have any effect with a web component.. so i'd argue we should just not have a css import in the web component examples to avoid confusion.

It isn't a pattern you will see, as the most you could do is style everything outside the component in that stylesheet. A web component would normally have its own shadow root, so those outside styles would have no effect on it (other than variables and host styles).

It isn't really a use case when dealing with web components because of this so i just want to make sure we're not including it just because non-web-component frameworks do it.

Example..

import './styles.css'; customElements.define('x-test', class XTest extends HTMLElement { // ... connectedCallback() { const h1 = document.createElement('h1'); h1.textContent = 'Hello!'; this.shadowRoot.appendChild(h1); } });

In this, it is impossible for anything in styles.css to ever style the h1. So that stylesheet can't possibly contain anything of use for the component. It would just be confusing to see that import when you have previously learnt that it isn't a thing anyone does/can do.

shilman

@tooppaaa LGTM but what's the resolution on those deleted CSS imports?

tooppaaa · 2020-09-08T09:46:40Z

@tooppaaa LGTM but what's the resolution on those deleted CSS imports?

Adding them back

shilman · 2020-09-13T08:07:02Z

@tooppaaa are you planning to add those CSS imports back? does the generated storybook look correct with them? does it look correct without them (can't imagine that it would)?

43081j · 2020-09-13T14:54:15Z

what is in those CSS files? where are they actually located? it would be helpful to know what they are trying to style...

i argued against them because its impossible to style the component from a separate, standalone stylesheet. due to how web components generally work... but if its just a stylesheet of the page and not the component, fair enough it should stay.

otherwise, though, you'd be including a stylesheet that does nothing. so i can't imagine its working right visually?

43081j · 2020-09-13T15:00:18Z

i've read through the code.

this is the CSS presumably:
https://github.com/storybookjs/storybook/blob/next/lib/cli/src/frameworks/common/page.css

in which case, its styling outer elements (light dom) so we should keep the import.

but this now raises another point... i assumed everything in cli/src/frameworks was like an "example". but none of these web-component "examples" (button, header, page) actually use web components... 🙈 is that what they are meant to be? where do they get used?

tooppaaa · 2020-09-14T08:05:44Z

@shilman Styles added back, all good to me ! Fix Typings as well to be exactly the same as React

@43081j Styles are shared for all frameworks. Even if this method of styling is not a web component standard, at the end, it's still HTML elements with class names. Styling applies as usual. They might not be reusable component for your application but the point here is to demonstrate the stories.
I'm not sure I get "none of these web-component "examples" (button, header, page) actually use web components... ".

It's possible that due to our knowledge on WebComponents, they might not be top of the art.
What do you think of moving on with this and create a new PR where you can improve these components ?

43081j · 2020-09-14T20:55:17Z

As in, the examples are definitely not making any use of web components at all.

Styles are shared for all frameworks. Even if this method of styling is not a web component standard, at the end, it's still HTML elements with class names

it isn't. if you style a web component (assuming the component has a shadow root), it is not "still HTML elements with class names". Your stylesheet, by definition, cannot penetrate the boundaries of the shadow root. It would be impossible to style anything within the shadow root from that style sheet.

I think this is a misunderstanding of web components, so i'll try explain.

Our example here is pretty much:

const button = (props) => html`<button ?disabled=${props.disabled}>Foo</button>`;

Something like that. That is not a web component and may as well live in the HTML examples rather than the web component ones.

Ultimately, that will produce a lit TemplateResult which just holds the template (<button>Foo</button>) and the interpolated parts (?disabled=${props.disabled}). This will just be rendered as regular HTML, the same as doing:

const button = (props) => `<button${props.disabled ? ' disabled' : ''}>Foo</button>`;

you see? the above would be seen as a HTML example, but its behaviourally the same as our "web component" example (because its not really using web components).

a real web component example would define button as this (or similar):

customElements.define('my-button', class extends LitElement {
  static get properties() {
    return { disabled: { type: Boolean } };
  }
  render() {
    // this button can never be styled by the outer CSS stylesheet. no selector can penetrate the boundaries
    // of the shadow root this ends up in.
    return html`<button ?disabled=${this.disabled}><slot></slot></button>`;
  }
});
const button = (props) => html`<my-button ?disabled=${props.disabled}>Foo</my-button>`;

in this case, our button "factory" is still ultimately returning some HTML (nothing to do with web components) but one of the elements within it is itself a web component.

if we wrote our examples like this, using actual web components, the stylesheet you're importing would not do anything.

because the section and everything else would live inside a shadow root, unstyleable by "light DOM" CSS (the stylesheet, imported from outside the web components isolated shadow root).

Example DOM tree:

// style.css
section { background: cyan; }

// DOM
div
- page-element (custom element)
-- #shadow-root
--- section (THIS SECTION WILL NOT BE CYAN)
---- p "some text"

edit:

im happy with just going ahead with this PR in the end as-is, but i think it would be very beneficial to rewrite these examples using web components. hopefully the explanation helps to show why. right now, they may as well be copies of the HTML examples (just without the html tagged template).

again i don't want this PR to hang about and go stale etc so we should try get it merged for sure. i hope the explanation helped either way

43081j · 2020-09-29T20:47:14Z

@tooppaaa would be good to get this finished off if you get chance soon. we should leave the stories and css as they are for now

43081j · 2020-10-24T10:33:01Z

@tooppaaa @shilman any chance we can pick this up again? looks like an easy one to merge now if we leave the css alone

ndelangen

if it works, 👍

43081j · 2020-12-17T08:52:12Z

not to be a bother but can we get this merged?

would be really useful to have these types 👍

ariefurtado · 2021-03-16T21:44:58Z

Hello @shilman,

I've been using the types from the @storybook/web-components, and I've found an issue with the types from lit-element.

As you can see in the image above, I am receiving an error about the 'lit-element' module.
After playing around a little bit, I was able to make it work when importing from lit-html instead of lit-element.

Can you reproduce this error? Do you agree with this change?

Reference to the types.ts file:
https://github.com/storybookjs/storybook/blob/next/app/web-components/src/client/preview/types.ts

43081j · 2021-03-17T07:48:43Z

It should probably be lit-html but there's other issues to consider too.

Storybooks usual dependency mismatch problem applies here too. There can only be one copy of lit in the entire dependency tree or instanceof bugs happen (fixed in lit-next).

Also your stories, components and storybook should import from the same thing, lit-html or lit-element but not a mixture.

Again not the problem you're seeing but would still exist as edge case issues so figured I'd mention them. Your fix is correct for this particular issue.

I'll try it today myself just to confirm too

shilman · 2021-03-17T09:11:58Z

cc @gaetanmaisse

43081j · 2021-03-17T09:21:01Z

@ariefurtado you are correct, if you could open a PR for it that would be great.

lit-element depends on lit-html, so it makes sense we pull it from the base package (lit-html) in case you don't use lit-element.

edit:

theres actually a fair amount of references to lit-element elsewhere,so im wondering if they all need changing or are just demo-related/example-related? something to take into account if doing a pr

edit2:

we actually do import lit-html in the sources here.

so the types should also import it. and when lit-next gets released eventually, we can drop the instanceof check too as that can cause version mismatch trouble.

gaetanmaisse · 2021-03-17T10:47:55Z

@ariefurtado and @43081j thanks for the feedback and details, doing a PR right now -> #14258

About:

Storybooks usual dependency mismatch problem applies here too. There can only be one copy of lit in the entire dependency tree or instanceof bugs happen (fixed in lit-next).

This shouldn't not happen as lit-html is a peer dep or @storybook/web-component so Storybook end up using the version defined in user's project. Except if you are messing things in our own project maybe 🤔 but as a lit-element user you will get a missing peer dep warning 😞 🤯

Edit:

theres actually a fair amount of references to lit-element elsewhere,so im wondering if they all need changing or are just demo-related/example-related? something to take into account if doing a pr

The examples/web-components-kitchen-sink project is using lit-element so I think it's ok on that side. However, our E2E tests for WC looks to install both lit-html and lit-element, I would think lit-element is enough no?

43081j · 2021-03-17T11:28:56Z

yes the e2e should only need lit-element.

and yes you are right about the peer dependency. the mismatch stuff is an issue outside storybook we need to take into account at a later date, unrelated to this issue. basically lit (and storybook in the file i referenced earlier) uses instanceof to know if the passed object is a lit template. if your dep tree has mixed versions, this can fail, so lit-next removes all instanceof checks and replaces them with a better assertion. once that lands, we will need another PR to do the same check in our code here

ariefurtado · 2021-03-17T15:13:51Z

Hi @43081j and @gaetanmaisse, I'm glad that I could help with this issue.

43081j and others added 4 commits August 14, 2020 16:15

add web-component story types

a4eb172

export common imports

9a96e22

move web component stories to typescript

e956b0b

CLI: add typescipt web components & types

aa23127

tooppaaa added maintenance User-facing maintenance tasks cli typescript web-components labels Sep 7, 2020

tooppaaa requested review from igor-dv, ndelangen, shilman and stijnkoopal as code owners September 7, 2020 06:29

tooppaaa changed the title ~~Pr/12026~~ CLI: Add typescript for web components and story types Sep 7, 2020

43081j reviewed Sep 7, 2020

View reviewed changes

remove duplicate types

c27ba99

tooppaaa commented Sep 7, 2020

View reviewed changes

cleanup

f335859

shilman reviewed Sep 8, 2020

View reviewed changes

tooppaaa added 2 commits September 14, 2020 09:30

Add styles back

bee5bb1

Fix typings

9009066

ndelangen approved these changes Oct 26, 2020

View reviewed changes

Merge branch 'next' into pr/12026

431d209

shilman added the run e2e extended test suite Run the e2e extended test suite in CircleCI workflows label Dec 20, 2020

shilman changed the title ~~CLI: Add typescript for web components and story types~~ Web-components: Add typescript types and update CLI Dec 20, 2020

Update types-6-0.d.ts

6853044

shilman changed the title ~~Web-components: Add typescript types and update CLI~~ Web-components: Add typescript types and update CLI template Dec 20, 2020

shilman changed the title ~~Web-components: Add typescript types and update CLI template~~ Web-components: Add typescript types and CLI template Dec 20, 2020

shilman added feature request and removed maintenance User-facing maintenance tasks labels Dec 20, 2020

Update types-6-0.ts

f674915

shilman modified the milestones: 6.2 core, 6.2 essentials Dec 20, 2020

shilman merged commit cc106bd into next Dec 20, 2020

shilman deleted the pr/12026 branch December 20, 2020 11:52

This was referenced Dec 20, 2020

Define Story for web-components in typings #11974

Closed

CLI: Typings for frameworks #11845

Open

43081j mentioned this pull request Mar 6, 2021

add web-component story types #12026

Closed

gaetanmaisse mentioned this pull request Mar 17, 2021

WebComponents: Fix types and improve CLI detection #14258

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Web-components: Add typescript types and CLI template #12395

Web-components: Add typescript types and CLI template #12395

tooppaaa commented Sep 7, 2020

43081j left a comment

43081j Sep 7, 2020

tooppaaa Sep 7, 2020

43081j Sep 7, 2020

43081j Sep 7, 2020

tooppaaa Sep 7, 2020

43081j Sep 7, 2020

tooppaaa Sep 7, 2020

43081j Sep 7, 2020

tooppaaa Sep 8, 2020

43081j Sep 8, 2020

tooppaaa Sep 7, 2020

43081j Sep 7, 2020 •

edited

Loading

43081j Sep 7, 2020 •

edited

Loading

tooppaaa Sep 8, 2020

43081j Sep 8, 2020 •

edited

Loading

shilman left a comment

tooppaaa commented Sep 8, 2020

shilman commented Sep 13, 2020 •

edited

Loading

43081j commented Sep 13, 2020

43081j commented Sep 13, 2020 •

edited

Loading

tooppaaa commented Sep 14, 2020

43081j commented Sep 14, 2020 •

edited

Loading

43081j commented Sep 29, 2020

43081j commented Oct 24, 2020

ndelangen left a comment

43081j commented Dec 17, 2020

ariefurtado commented Mar 16, 2021

43081j commented Mar 17, 2021 •

edited

Loading

shilman commented Mar 17, 2021

43081j commented Mar 17, 2021 •

edited

Loading

gaetanmaisse commented Mar 17, 2021 •

edited

Loading

43081j commented Mar 17, 2021

ariefurtado commented Mar 17, 2021

		@@ -0,0 +1,33 @@
		import { html } from 'lit-html';

		export interface ButtonProps {

Web-components: Add typescript types and CLI template #12395

Web-components: Add typescript types and CLI template #12395

Conversation

tooppaaa commented Sep 7, 2020

What I did

How to test

43081j left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

43081j Sep 7, 2020 • edited Loading

Choose a reason for hiding this comment

43081j Sep 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

43081j Sep 8, 2020 • edited Loading

Choose a reason for hiding this comment

shilman left a comment

Choose a reason for hiding this comment

tooppaaa commented Sep 8, 2020

shilman commented Sep 13, 2020 • edited Loading

43081j commented Sep 13, 2020

43081j commented Sep 13, 2020 • edited Loading

tooppaaa commented Sep 14, 2020

43081j commented Sep 14, 2020 • edited Loading

43081j commented Sep 29, 2020

43081j commented Oct 24, 2020

ndelangen left a comment

Choose a reason for hiding this comment

43081j commented Dec 17, 2020

ariefurtado commented Mar 16, 2021

43081j commented Mar 17, 2021 • edited Loading

shilman commented Mar 17, 2021

43081j commented Mar 17, 2021 • edited Loading

gaetanmaisse commented Mar 17, 2021 • edited Loading

43081j commented Mar 17, 2021

ariefurtado commented Mar 17, 2021

43081j Sep 7, 2020 •

edited

Loading

43081j Sep 7, 2020 •

edited

Loading

43081j Sep 8, 2020 •

edited

Loading

shilman commented Sep 13, 2020 •

edited

Loading

43081j commented Sep 13, 2020 •

edited

Loading

43081j commented Sep 14, 2020 •

edited

Loading

43081j commented Mar 17, 2021 •

edited

Loading

43081j commented Mar 17, 2021 •

edited

Loading

gaetanmaisse commented Mar 17, 2021 •

edited

Loading