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

Component is not re-rendered when its iternal state is updated. #12578

Open
adarean5 opened this issue Sep 25, 2020 · 23 comments
Open

Component is not re-rendered when its iternal state is updated. #12578

adarean5 opened this issue Sep 25, 2020 · 23 comments

Comments

@adarean5
Copy link

adarean5 commented Sep 25, 2020

Describe the bug
I am working on a Storybook project that uses lit-element based components.
While creating the Storybook project with the sb init script I selected the Web Components option.

I imported one of my components, that is using lit-element and got it working for the most part.
The component updates when I change the controls in the storybook UI Storybook and so on.

The component doesn't re-render if its props change internally.
For instance: I have a counter component that is composed of a button and a text element.
The component has a prop count, which is updated every time the button is clicked via a component's function called increment which just increases the count by one.

As far as I could tell, the button click will trigger the increment function properly, the count prop will increase, but the component won't be re-rendered with the new value of the count prop.

This also happened when I modified the original Button component that came with the Web Components storybook, which only uses lit-html.

To Reproduce
Steps to reproduce the behavior:

  1. Create a new storybook project for Web Components.
  2. Create ExampleComponent.ts with the source code below.
  3. Register the component in preview.js with the code below.(I am not sure if this is the right way to do it, but it seems that defining the component inside the .stories.ts file can cause errors on hot reload).
  4. Create the ExampleComponent.stories.ts file (use the code below).
  5. Run the storybook script.
  6. Select the Example component story from the side menu.
  7. Click the "+" button on the component next to the "Counter value: 10" text.

Expected behavior
The number next to the button should update.

Screenshots
This is how the component should look in the UI
image

Code snippets
The lit element component:

// ExampleComponent.ts

import { html, css, LitElement, property, TemplateResult } from 'lit-element';
/**
 * @element example-component
 * @prop title - Title of the component
 * @prop counter - By how much the component was incremented
 * @fires example-event - This event emits the current value of the counter property.
 */

export class ExampleComponent extends LitElement {
    static styles = css`
        :host {
            display: block;
            padding: 25px;
            color: #000000;
        }
    `;

    @property({ type: String }) title = 'Example counter';

    @property({ type: Number }) counter = 0;

    __increment(): void {
        this.counter++;
        console.log('Counter value', this.counter);

        const event = new CustomEvent('example-event', {
            detail: {
                counter: this.counter,
            },
        });
        this.dispatchEvent(event);
    }

    render(): TemplateResult {
        return html`
            <h2>${this.title}</h2>
            <span>Counter value: ${this.counter}</span>
            <button @click=${this.__increment}>+</button>
        `;
    }
}

I define the custom element in .storybook/preview.js like so:

import { ExampleComponent } from ... ;

const componentName = 'indoc-example-component';

function defineCustomElement(): void {
    customElements.get(componentName) || customElements.define(componentName, ExampleComponent);
}

defineCustomElement();

And this is how the storybook file is defined:

// ExampleComponent.stories.ts

import { ExampleComponent } from ... ;

export default {
    title: `Example / Example component`,
    component: 'indoc-example-component',
    argTypes: {
        __increment: { action: '@click' },
    },
};

const Template = (args: any) =>
    `<indoc-example-component title="${args.title}" counter="${args.counter}"></indoc-example-component>`;

export const Primary: any = Template.bind({});
Primary.args = {
    title: 'Click the counter to increment',
    counter: 10,
};

System:

Environment Info:

  System:
    OS: Windows 10 10.0.18363
    CPU: (12) x64 Intel(R) Core(TM) i7-8750H CPU @ 2.20GHz
  Binaries:
    Node: 12.18.3 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.4 - C:\Program Files (x86)\Yarn\bin\yarn.CMD
    npm: 6.14.5 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Chrome: 85.0.4183.102
    Edge: Spartan (44.18362.449.0)

Additional context
I am using version 6.0.21 of Storybook.
Before switching to standard Storybook I was using the @open-wc/demoing-storybook (https://www.npmjs.com/package/@open-wc/demoing-storybook) package for demoing my lit-element based components, and the component updated as expected.
The component also behaves as expected when used inside a Vue 3 project.

@shilman
Copy link
Member

shilman commented Sep 25, 2020

Do you have a repro repo you can share?

@shilman
Copy link
Member

shilman commented Sep 25, 2020

@daKmoR @LarsDenBakker any idea why this might work in the open-wc version but fail in storybook proper?

@adarean5
Copy link
Author

Do you have a repro repo you can share?

Not at the moment unfortunately, the code is part of a larger project that is not open source, but I will try and create a repro repo on Monday.

@adarean5
Copy link
Author

I created a repro repo: https://github.com/adarean5/storybook-web-components-repro
It is a standard storybook project for Web components, the code that I added for minimal repro can be seen in this commit

@adarean5
Copy link
Author

I also realize this issue might have occurred due to user error, but I was unable to find any resources on how to properly configure storybook for lit-element based web components.

@shilman
Copy link
Member

shilman commented Sep 28, 2020

Thanks so much for creating a repro -- hopefully we can get to the bottom of this! 🙏

@LarsDenBakker
Copy link
Contributor

LarsDenBakker commented Sep 28, 2020

This might be related to https://github.com/Polymer/lit-element/issues/1030

When using typescript decorators, we need to make sure class fields also get compiled correctly. The new standard based class fields use "define" semantics for class fields, while typescript decorators rely on "set" semantics class fields.

So we need to make sure the class fields are compiled by TS/babel using this logic. I'm not 100% up to date with what happens where in the pipeline of storybook. You could try setting target to es2020 or es2019 in your tsconfig.

Separately it might be helpful in storybook to configure it to compile class fields this way when people use typescript, since that's what a lot of typescript users rely on.

@shilman
Copy link
Member

shilman commented Sep 28, 2020

@adarean5 any chance you can investigate these suggestions and report back? i'm open to changing Storbyook's TS config if it doesn't break anything else.

@adarean5
Copy link
Author

Firstly, @LarsDenBakker thank you for the suggestion!
@shilman I will, but unfortunately I will not have much free time this week, so it might take a while. Hope that's ok.

@adarean5
Copy link
Author

adarean5 commented Sep 29, 2020

@shilman luckily I did manage to find some time to look at what @LarsDenBakker suggested.
Overloading the connectedCallback method of the component seems to resolve the issue.

connectedCallback() {
        // @ts-ignore
        this._saveInstanceProperties();
        super.connectedCallback();
    }

However I didn't want to modify my component just for Storybook, and I was also worried about this quote from the issue that @LarsDenBakker linked:

We don't want to implement this generally because it's highly likely to introduce a significant performance penalty. The idea is that when you lazy load your definition, the performance penalty of modifying the class is offset by the benefit of loading the definition lazily.

So I decided to extend my component in the .story file and overload it there instead. You can see the changes that were required in this commit (excuse the unnecessary .idea files, forgot to update the gitignore
😅).

I didn't have the chance to test it thoroughly, but on the surface it seems to do the trick.
It does seem like a bandage fix though, and it adds a bit of boilerplate to the project.

@shilman
Copy link
Member

shilman commented Sep 29, 2020

Awesome you found a workaround! @LarsDenBakker Any idea what the proper storybook fix might look like?

@LarsDenBakker
Copy link
Contributor

LarsDenBakker commented Sep 29, 2020

I dug into this a bit, it looks like class properties are compiled correctly using "loose" mode:

[require.resolve('@babel/plugin-proposal-class-properties'), { loose: true }],
which makes it compatible with decorating class fields.

But unfortunately babel decorators don't work quite the same as typescript, and in this case the lit-element decorators don't work with babel legacy decorators: lit/lit-element#205. They do work with typescript decorators, and non-legacy babel decorators.

Quite a messy situation with all these competing implementations of non-standard syntax :)

In my opinion if you rely on typescript semantics other than types, you should compile with TSC before doing anything with the code. Otherwise you'll always be chasing tools to get the same behavior. You may also be able to overwrite the babel configuration of storybook, for example to use the non-legacy decorator plugin.

@Niznikr
Copy link

Niznikr commented Sep 29, 2020

To have TS Lit components working in our Storybook we set plugin-proposal-decorators to this (which also keeps the legacy option to false by default):
'@babel/plugin-proposal-decorators', { decoratorsBeforeExport: true }

@Niznikr
Copy link

Niznikr commented Sep 29, 2020

Our full Storybook babel config if that's helpful:

module.exports = {
  plugins: [
    '@babel/plugin-transform-shorthand-properties',
    '@babel/plugin-transform-block-scoping',
    ['@babel/plugin-proposal-decorators', { decoratorsBeforeExport: true }],
    ['@babel/plugin-proposal-class-properties', { loose: true }],
    ['@babel/plugin-proposal-private-methods', { loose: true }],
    '@babel/plugin-proposal-export-default-from',
    '@babel/plugin-syntax-dynamic-import',
    ['@babel/plugin-proposal-object-rest-spread', { loose: true, useBuiltIns: true }],
    '@babel/plugin-transform-classes',
    '@babel/plugin-transform-arrow-functions',
    '@babel/plugin-transform-parameters',
    '@babel/plugin-transform-destructuring',
    '@babel/plugin-transform-spread',
    '@babel/plugin-transform-for-of',
    'babel-plugin-macros',
    '@babel/plugin-proposal-optional-chaining',
    '@babel/plugin-proposal-nullish-coalescing-operator',
    ['babel-plugin-emotion', { sourceMap: true, autoLabel: true }],
  ],
  presets: [
    ['@babel/preset-env', { shippedProposals: true, useBuiltIns: 'usage', corejs: '3' }],
    '@babel/preset-typescript',
  ],
};

@shilman
Copy link
Member

shilman commented Sep 30, 2020

Unfortunately SB's built-in typescript support is based on babel. We currently don't have a tsc option, tho we'll probably add a "disable built in" option at some point.

@omar-belkhodja-bacdev
Copy link

Hello guys. I think I'm facing the same issue using the project pwa-webpack-starter-kit. If you run the project using npm install && npm run start everything will work OK. If you change the babel configuration from
['@babel/proposal-decorators', { decoratorsBeforeExport: true, }],
to
['@babel/proposal-decorators', { legacy: true, }],
The elements won't be rendered.

@LarsDenBakker
Copy link
Contributor

LarsDenBakker commented Sep 30, 2020

I think it would be useful to make this a bit more configurable in storybook. I understand the desire to make things easy for users by default, but having plugins enabled for non-standard features can be problematic for people when proposals change over time and browsers start to implement the final standardized version. Just a suggestion of course :)

@shilman
Copy link
Member

shilman commented Oct 1, 2020

@LarsDenBakker I agree. In general we'd try to give users three options:

  • Use our recommended default
  • Refine our default (e.g. override babel options)
  • Disable our default and handle it yourself

Hopefully Coming Soon (TM)

adarean5 referenced this issue in adarean5/storybook-web-components-repro Oct 2, 2020
@adarean5
Copy link
Author

adarean5 commented Oct 2, 2020

Thank you for sharing the config that solved the problem for you @Niznikr , it allowed me to come up with my own solution that uses the babel option in .storybook/main.js.

Here is the code snippet that I added to module.exports in .storybook/main.js:

 babel: async (options) => {
    Object.assign(options.plugins.find((plugin) => plugin[0].includes('plugin-proposal-decorators'))[1], {
      decoratorsBeforeExport: true,
      legacy: false
    })
    return options;
  }

It finds the existing config for '@babel/proposal-decorators' and modifies the properties, so they work with lit-element's decorators.

You can find the full solution in this commit in the main.js file.

I'm sure the solution could be improved upon and I would be thankful for any suggestions, but it seems to work well enough for now and unlike my previous solution it does not require any special boilerplate in the .story files.

I would also like to thank everyone for their help and suggestions!

@stale
Copy link

stale bot commented Dec 26, 2020

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Dec 26, 2020
@Arithmetics
Copy link

Thank you for sharing the config that solved the problem for you @Niznikr , it allowed me to come up with my own solution that uses the babel option in .storybook/main.js.

Here is the code snippet that I added to module.exports in .storybook/main.js:

 babel: async (options) => {
    Object.assign(options.plugins.find((plugin) => plugin[0].includes('plugin-proposal-decorators'))[1], {
      decoratorsBeforeExport: true,
      legacy: false
    })
    return options;
  }

It finds the existing config for '@babel/proposal-decorators' and modifies the properties, so they work with lit-element's decorators.

You can find the full solution in this commit in the main.js file.

I'm sure the solution could be improved upon and I would be thankful for any suggestions, but it seems to work well enough for now and unlike my previous solution it does not require any special boilerplate in the .story files.

I would also like to thank everyone for their help and suggestions!

this worked for me (2021-10-06) if youre coming here trying to get this to work

@stale stale bot removed the inactive label Oct 6, 2021
@aquinoWill
Copy link

Thank you for sharing the config that solved the problem for you @Niznikr , it allowed me to come up with my own solution that uses the babel option in .storybook/main.js.
Here is the code snippet that I added to module.exports in .storybook/main.js:

 babel: async (options) => {
    Object.assign(options.plugins.find((plugin) => plugin[0].includes('plugin-proposal-decorators'))[1], {
      decoratorsBeforeExport: true,
      legacy: false
    })
    return options;
  }

It finds the existing config for '@babel/proposal-decorators' and modifies the properties, so they work with lit-element's decorators.
You can find the full solution in this commit in the main.js file.
I'm sure the solution could be improved upon and I would be thankful for any suggestions, but it seems to work well enough for now and unlike my previous solution it does not require any special boilerplate in the .story files.
I would also like to thank everyone for their help and suggestions!

this worked for me (2021-10-06) if youre coming here trying to get this to work

This is work for too, thank you!

@shilman shilman removed the PN label Apr 24, 2023
@s0n4
Copy link

s0n4 commented May 31, 2023

Hi! I have the same issue using the new storybook 7 with vite-builder. On a static build the story does not re-render when updating the model. Any idea if the solution could be similar?

env:
Vue 2.7.14
Storybook 7.0.18
Vuetify 2.6.15
vite 4.3.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants