Storybook 6.2.0-beta new angular renderer ovverides properties

kbrilla commented 3 years ago

Describe the bug if component has some properties that are not inputs they are overridden

export class SampleComp{
   @Input() input1: string;
   notInputProperty: string;

  ngOnChanges(){
  this.notInputProperty = 'kokos';
   }
}

and template will be

notInputProperty : {{notInputProperty }}

then notInputProperty will be overridden after change detection we will get rendered ``notInputProperty :.

if we add @Input decorator to this property then it works as it should and we get rendered: ```notInputProperty : kokos'.

setting angularLegacyRendering: true, solves the issue.

export default {
  title: 'Components/Core/SampleComp',
  component: SampleComp,
  decorators: [
    moduleMetadata({
      declarations: [SampleComp],
    }),
  ],
} as Meta;

const Template: Story<SampleComp> = (args) => ({
  props: args,
});

export const Sample = Template.bind({});
Sample.args = {};

fun fact: after first time storybooks renders You will change the name of property to lets say notInputProperty2 then it will work! So maybe it has something to do with controls? Restarting storybook (ctr+c) and npm run storybook again will again cause it to be overridden till You again change the name of property to lets say notInputProperty3.

System

  System:
    OS: Windows 10 10.0.19042
    CPU: (8) x64 Intel(R) Core(TM) i5-8265U CPU @ 1.60GHz
  Binaries:
    Node: 14.16.0 - C:\Program Files\nodejs\node.EXE
    npm: 6.14.11 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Chrome: 88.0.4324.190
    Edge: Spartan (44.19041.423.0), Chromium (88.0.705.81)
  npmPackages:
    @storybook/addon-actions: 6.2.0-beta.10 => 6.2.0-beta.10
    @storybook/addon-centered: 5.3.21 => 5.3.21
    @storybook/addon-essentials: 6.2.0-beta.10 => 6.2.0-beta.10
    @storybook/addon-links: 6.2.0-beta.10 => 6.2.0-beta.10
    @storybook/addon-storysource: 6.2.0-beta.10 => 6.2.0-beta.10
    @storybook/angular: 6.2.0-beta.10 => 6.2.0-beta.10

ThibaudAV commented 3 years ago

Yes, it's true. But I don't think it's a "bug". 🙈 (but a change in behaviour that I would also like to make). Give me a few days to find time to make a complete answer. (very busy lately 😞 )

kbrilla commented 3 years ago

Well I don’t think storybook should change in any way properties that are not inputs and are even private (i know ts private is come and take It). But, sure will wiat for update on Your side. For now will keep legacy rendering on.

W dniu śr., 10.03.2021 o 08:48 ThibaudAV @.***> napisał(a):

Yes, it's true. But I don't think it's a "bug". 🙈 Give me a few days to find time to make a complete answer. (very busy lately 😞 )

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/storybookjs/storybook/issues/14147#issuecomment-795037493, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFKBQQJZ3Y24YTJABFG6NVDTC4P5DANCNFSM4YVC2VIQ .

ThibaudAV commented 3 years ago

In fact I don't know where to start... many things are mixed up

Problem Replication:

If I understand correctly your example is this:

Click to expand!

```ts import { Story, Meta, moduleMetadata } from '@storybook/angular'; import { Input, Component } from '@angular/core'; @Component({ selector: 'foo', template: 'notInputProperty : {{notInputProperty }}' }) class SampleComp { @Input() input1: string; notInputProperty = 'rr'; ngOnChanges() { this.notInputProperty = 'kokos'; } } export default { title: 'Legacy / angularLegacyRendering', component: SampleComp, decorators: [ moduleMetadata({ declarations: [SampleComp], }), ], } as Meta; const Template: Story = (args) => { console.log(args); return { props: args, // angularLegacyRendering: true, }; }; export const Sample = Template.bind({}); Sample.args = {}; ```

here are the steps I do :

Add story without angularLegacyRendering: true
run yarn storybook

see the console.log {input1: undefined, notInputProperty: undefined, ngOnChanges: undefined}
change input1 value with controls

nothing happen but i have log + error in console :
change with angularLegacyRendering: true

see the console.log {} :wow: empty now
change input1 value with controls

see the console.log {input1: "s"} It works, we can see Koko in canvas

Now, if i do same step (close storybook client and restart it) but i start storybook with angularLegacyRendering: true

I have the same behavior. So, If it's the same problem you encountered, the problem is not the new rendering I'm not mistaken? 🙏

I think the problem comes from compodoc. Which discovers all properties of your component class and adds them to agrs. If you don't control it, it will do anything.

Discussions are already in progress to replace compodoc https://github.com/storybookjs/storybook/issues/12689 https://github.com/storybookjs/storybook/issues/8672

personally I don't like compodoc at all and I don't use it

I recommend you not to trust compodoc if you use it and to do something like :

const Template: Story<SampleComp> = (args) => ({
  props: {
  input1: args.input1
  },
});

to be sure to send only the expected "arg"

So :

compodoc reference private properties

Wait for an alternative. or manually add controls of args

props can override private (or non @input) properties of components

Several topics are talking about it, it also blocks some features I have already made a response in some of them

https://github.com/storybookjs/storybook/issues/11613 https://github.com/storybookjs/storybook/issues/12946

or else https://github.com/storybookjs/storybook/issues/12438

I really wish I could ban it or split it into 2 like: props : only angular Input / Output / directive (ngClass,...) or html attributs (class,...) overrideComponentProperties: As before allows to override a property of the component class

but this is a big breaking changes I think and we have to wait for a major version (I was told) 😁

fun fact :

fun fact: after first time storybooks renders You will change the name of property to lets say notInputProperty2 then it will work! So maybe it has something to do with controls?

not with controls with compodoc. As you do not completely reload the storybook client compodoc does not reanalyze your component

legacy rendering and ngOnChanges

I don't recommend to use legacy rendering if you have stories and component with ngOnchanges. it is manually called by the renderer and is not exactly the same as native angular. see: https://github.com/storybookjs/storybook/blob/a8770a4a8e69b441b56cc3c2a1e906e913e0907c/app/angular/src/client/preview/angular/components/app.component.ts#L119

The new renderer doesn't work the same way because it simulates a template for your component and therefore lets the native angular mechanism do the work

nicolae536 commented 3 years ago

A possible fix until the main issue gets fixed you can disable compodoc in main.js. But as a result you have to declare the argTypes on all components and maintain it.

// Because of the following issues we cannot use Compodoc to automatically find component inputs:
// https://github.com/storybookjs/storybook/issues/14147
// https://github.com/storybookjs/storybook/issues/11613
// It seems storybook will try to add support for detecting inputs because compodoc is not maintained anymore and they end up overriding angular component
// props which are not imputs with ng versions bigger than 8.0
// When this is closed we can probably see what's the upgrade path: https://github.com/storybookjs/storybook/issues/8672
// import { setCompodocJson } from "@storybook/addon-docs/angular";
import { INITIAL_VIEWPORTS } from '@storybook/addon-viewport';
// import docJson from "../documentation.json";
// setCompodocJson(docJson);

export const parameters = {
  actions: { argTypesRegex: "^on[A-Z].*" },
  controls: {
    matchers: {
      color: /(background|color)$/i,
      date: /Date$/,
    },
  },
  viewport: {
    viewports: INITIAL_VIEWPORTS,
  },
}

salmin89 commented 3 years ago

@ThibaudAV

legacy rendering and ngOnChanges

I don't recommend to use legacy rendering if you have stories and component with ngOnchanges. it is manually called by the renderer and is not exactly the same as native angular. see:

We turned on legacyRendering because we got these kinds of errors:

ERROR TypeError: provider.ngAfterViewInit is not a function

How would we turn off angularLegacyRendering and solving the above error?

ThibaudAV commented 3 years ago

@salmin89 can you give more details about the component you added in the story ?

Make sure that you do not directly pass all args in your story props prefer a "destructuring" to control them :

const Template: Story<SampleComp> = (args) => ({
  props: {
  input1: args.input1
  },
});

ThibaudAV commented 3 years ago

I think we can close it a workaround has been added to avoid the override can you check if the problem is still present with the last version 🙏

DigiBanks99 commented 2 years ago

Hi, the problem still persists and the workaround also doesn't work anymore.

Storybook: v6.50-alpha.14 Compodoc: v1.1.18 Angular: v13

pscadding commented 2 years ago

In case it helps anyone else who ends up here I added --disablePrivate, --disableInternal and --disableLifeCycleHooks to the storybook builder compodocArgs options to get it to stop finding the private properties. Added in the angular.json:

{
  ...
  "projects": {
    "my-lib": {
      ...
      "architect": {
        "storybook": {
          "builder": "@storybook/angular:start-storybook",
          "options": {
            ...
            "compodocArgs": ["-e", "json", "--disablePrivate", "--disableInternal", "--disableLifeCycleHooks"]
          }
        },

Another workaround that I don't like as much is to set the parameters on my story to either exclude or include the properties from the controls.

Primary.parameters = {
  controls: {
    // either exclude or include
    exclude: ['ngAfterContentInit', 'display'],
    include: ['percentage', 'error', 'status']
  },
}

storybookjs / storybook