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

Angular: Overhaul preview renderer #13215

Merged
merged 7 commits into from Dec 3, 2020

Conversation

ThibaudAV
Copy link
Contributor

@ThibaudAV ThibaudAV commented Nov 22, 2020

Issue: #11614

Work in progress, TODO list:

  • fix Custom/Feature Module as Context story
  • add more test for Service

I did not browse all issues. but it fixes, for instance, the color background which does not change on this issue:
#11614

What I did

Two main goals:

1. make operation of angular rendering more clear

(currently, all the functions are tangled in helpers file or ) and does not make the code very clear to understand how it works

2. use a new way to add story component|template in the Angular application

Currently the component | template of the story is added dynamically with componentFactoryResolver. This way does not add the component in the same way as if it was used in a template (the component will not have the same life cycle) which forces Storybook to complete the missing life cycle. (see app.component ngOnChanges, writeValue, registerOnChange, missing registerOnTouched, ext..)

This new approach no longer uses dynamic components.

  • For template: it's added in a component that will be used as a bootstrap component directly. All story properties are added as properties of the new wrapper component. (see this function)
  • For component: a template is generated representing the typical use of this component in a real template. This allows Angular to integrate it completely in its life cycle. (see this function)

So in this way Storybook should better simulate the real use of the story in the angular application.

Tests

I've checked all the story examples for angular (don't hesitate to do the same again, I think)
I found only one case that is problematic and that creates breaking changes

For the story Component with default providers and Component with overridden provider inside Custom/Feature Module as Context the ChipComponent cannot be added because it is not exported by ChipsModule and it is already declared by ChipsModule. So the virtual Storybook module cannot use it. :/
Before that, it was not a problem to dynamically add a non-exported component. Now, it's not possible anymore.

It is always possible to add a component already declared in a module because Storybook detects it and will not re-declare it.

Notes

Another point is on NO_ERRORS_SCHEMA, it would be really nice if addon doc + controls only add the Input and Output properties of a component, and so not include private properties anymore.

Removing this will allow Storybook to return an error if component inputs & outputs no longer match story inputs & outputs. Currently, there are no errors (if I am not wrong) but I think it would be nice if Storybook returns an error in this case. This will keep the story and associated components consistent.
With my current work, it would be very handy 🙈

I think the problem with this issue #11613 is the same
The doc+controls addon returns the value of the "private" property value to in story props.

On my 2nd commit, I don't know how to do otherwise without editing jest configuration to run a test with jest-preset-angular 🤔

How to test

  • Is this testable with Jest or Chromatic screenshots?

yes

  • Does this need a new example in the kitchen sink apps?

🤷‍♂️ maybe add this ? 0670442

  • Does this need an update to the documentation?

🤷‍♂️

👋 Don't hesitate to give me feedback, if you think it's not interesting. If the code is unclear or if there are too many comments or missing comments, ty

p.s : sorry for my bad english 🙈

@shilman
Copy link
Member

shilman commented Nov 23, 2020

@ThibaudAV can you explain the goals of the rewrite?

@ThibaudAV
Copy link
Contributor Author

yes @shilman I wanted to check with CI . but I plan to do it azap

@shilman
Copy link
Member

shilman commented Nov 23, 2020

cc @kroeder @Marklb

@ThibaudAV ThibaudAV force-pushed the new-angular-client branch 3 times, most recently from 0f35630 to bcc6277 Compare November 23, 2020 17:19
@gaetanmaisse gaetanmaisse added angular in progress maintenance User-facing maintenance tasks labels Nov 23, 2020
@Marklb
Copy link
Member

Marklb commented Nov 23, 2020

This looks like what I have been considering, so I will be going through it later(hopefully this afternoon) and seeing how it works.

I have only looked at the code so far, but I will list questions/suggestions I have so far.

  • I understand wanting to only have the inputs/outputs in props, but I don't see that as a possibility, because the props are not specific to the component. Some of my stories use props that go with multiple or none of the components, because the story is either showing how multiple components are used together or the component itself is rendered by my code, like my stories showing how breadcrumbs work and only specify <router-outlet></router-outlet> as the template.

    • Would it be possible to get the component's factory to filter it's inputs/outputs? Without Ivy, the component may need to be an entryComponent, but that shouldn't be an issue much longer I hope. I will check this when I get time to play with the code, but I think it would still be possible with this implementation.
  • Do you think this would have any effect on the aliased inputs/outputs? I don't think it helps, but would probably prevent unintentionally overwriting variables that aren't an input/output, since they are being handled by the template. I explained my issues trying to solve the problem here: The Action args emit wrong handler #12854 (comment)

  • This is what I was proposing as a way to support directives in stories. Do you think it would be reasonable to add that support to this? Related issue: Set ngClass/ngStyle from props #12946.

  • As for ng-content, with the existing implementation, I was proposing projectableNodes as a possible solution, but if the template string creation was it's own utility function, could we iterate an array of components and use @ViewChildren, instead of @ViewChild, to still support component content with this implementation? Related issue: [Angular] Can't seem to generate a story with ng-content and knob-passed props #10272

  • You are using a BehaviorSubject instead of the ReplaySubject, but I assume it still has the NgZone problem. I will confirm this later, but I have a PR to hopefully fix that and you should be able to add/merge it into your implementation easily, if that solution is merged. If you want to try it out, I think it is ready to merge, but would like someone else to look it over.

  • Do you think we could improve Storybook's decorator support for Angular? Related issue: Documentation: angular decorator not working without template #12749

@ThibaudAV ThibaudAV force-pushed the new-angular-client branch 4 times, most recently from bb4d593 to 6f0bd1e Compare November 23, 2020 23:33
@ThibaudAV
Copy link
Contributor Author

ThibaudAV commented Nov 24, 2020

I have corrected some errors I made when creating the test 😅. now is ok in netlify

I understand wanting to only have the inputs/outputs in props, but I don't see that as a possibility, because the props are not specific to the component. Some of my stories use props that go with multiple or none of the components, because the story is either showing how multiple components are used together or the component itself is rendered by my code, like my stories showing how breadcrumbs work and only specify as the template.

I speak only in component case. In template case there is no problem.
For this issue I would think more that it should be a configuration of concerned story. By default, there is an error if the props does not respect component input/output contract. And if this is the wish of the story, just add the shemas NO_ERRORS_SCHEMA or another boolean. But don't let this be the default case 🤷‍♂️

But I let you choose :) I probably don't have all the context

Would it be possible to get the component's factory to filter it's inputs/outputs? Without Ivy, the component may need to be an entryComponent, but that shouldn't be an issue much longer I hope. I will check this when I get time to play with the code, but I think it would still be possible with this implementation.

haha I thought about it, but I think it would be a loss in performance. Load the component twice to be able to filter on properties :/

Do you think this would have any effect on the aliased inputs/outputs? I don't think it helps, but would probably prevent unintentionally overwriting variables that aren't an input/output, since they are being handled by the template. I explained my issues trying to solve the problem here: #12854 (comment)

Completely. As we get as close as possible to a real use of a component (I hope) this should solve this problem in part.
There is still the problem, for me, of the doc that automatically adds the local properties (which are not @Input/ @output ) of the component
If the doc addon is smart enough to find the alias and not add the local property it would be great!
(If I understand correctly)

This is what I was proposing as a way to support directives in stories. Do you think it would be reasonable to add that support to this? Related issue: #12946.

With this solution it works without any problems. If my quick test is correct

As for ng-content, with the existing implementation, I was proposing projectableNodes as a possible solution, but if the template string creation was it's own utility function, could we iterate an array of components and use @ViewChildren, instead of @ViewChild, to still support component content with this implementation? Related issue: #10272

Not sure to understand exactly 🤔
But if we are talking about the combined use of component + template to add template in the ng-content? I think it's possible. I will propose to do it in another PR if this one if validated? This solution would even facilitate this feature, I think.

You are using a BehaviorSubject instead of the ReplaySubject, but I assume it still has the NgZone problem. I will confirm this later, but I have a PR to hopefully fix that and you should be able to add/merge it into your implementation easily, if that solution is merged. If you want to try it out, I think it is ready to merge, but would like someone else to look it over.

What's the problem? we can use another observable one. it's just that this one made more sense to me for this situation.

Do you think we could improve Storybook's decorator support for Angular? Related issue: #12749

If I understand correctly. Yes, I don't think this solution closes doors for these features. On the contrary, I think it could make these improvements easier. What do you think about it ? I would be interested in trying to do it but in another PR, no ?

I can see something like that 🤔

export default {
  component: YourComponent,
  decorators: [
   // directly return the template according to whether it is a template | component (or both)
   templateDecorator((story) => `<div style="margin: 3em">${story}</div>`)
  ],
} as Meta;

@ThibaudAV ThibaudAV force-pushed the new-angular-client branch 2 times, most recently from 9f33c20 to 1c04762 Compare November 24, 2020 09:53
@Marklb
Copy link
Member

Marklb commented Nov 24, 2020

I speak only in component case. In template case there is no problem.
For this issue I would think more that it should be a configuration of concerned story. By default, there is an error if the props does not respect component input/output contract. And if this is the wish of the story, just add the shemas NO_ERRORS_SCHEMA or another boolean. But don't let this be the default case 🤷‍♂️

I could be over thinking this and may need additional feedback from other to see what would be expected. It may be configurable globally using the moduleMetadata decorator in preview.ts, which would make it simple for the user to decide their default. One situation I can think of that could cause additional props could be a knob/control added to toggle a value in a decorator. As long as it is obvious that NO_ERRORS_SCHEMA is needed to fix that, then it's no problem. Would user's that don't often write tests, or uses a library that handles the boilerplate, know that NO_ERRORS_SCHEMA exists? Would we be able to determine if that should be suggested without checking the inputs/outputs on the factory?

haha I thought about it, but I think it would be a loss in performance. Load the component twice to be able to filter on properties :/

Completely. As we get as close as possible to a real use of a component (I hope) this should solve this problem in part.
There is still the problem, for me, of the doc that automatically adds the local properties (which are not @Input/ @output ) of the component
If the doc addon is smart enough to find the alias and not add the local property it would be great!
(If I understand correctly)

It's been a while since I looked at what resolveComponentFactory does, but I thought it was basically just a lookup for the factory that creates the component. The inputs/outputs are on the factory object, so the component doesn't need to be created. I would need to look into it more to see what the impact would be, but I am not sure if there is another way to reliably get the templateName of inputs/outputs currently.

What's the problem? we can use another observable one. it's just that this one made more sense to me for this situation.

I forgot to reference the PR: #12382
The problem that I am fixing with that issue is the observable not always emitting in the zone, since an event triggered by anything outside our story, such as addons, are not hooked by NgZone. I just tried a story on your branch and it does have the problem. As long as that PR doesn't have a problem I am not seeing, it is a simple fix.

@ThibaudAV
Copy link
Contributor Author

I forgot to reference the PR: #12382
The problem that I am fixing with that issue is the observable not always emitting in the zone, since an event triggered by anything outside our story, such as addons, are not hooked by NgZone. I just tried a story on your branch and it does have the problem. As long as that PR doesn't have a problem I am not seeing, it is a simple fix.

I propose you an alternative solution in PR #12382
Whatever the choice I think that both are compatible with this pr no ?

@ThibaudAV
Copy link
Contributor Author

For input/output recovery, I can propose you this solution which doesn't require the angular engine and is based on Typescript. I don't think the Input/Output convention will change for a long time. what do you think about it ?

      const getComponentIOPropertiesName = (
        compType: any
      ): { inputs: string[]; outputs: string[] } | undefined => {
        const decoratorKey = '__prop__metadata__';
        const propsDecorators: Record<string, any[]> =
          Reflect &&
          Reflect.getOwnPropertyDescriptor &&
          Reflect.getOwnPropertyDescriptor(compType, decoratorKey)
            ? Reflect.getOwnPropertyDescriptor(compType, decoratorKey).value
            : compType[decoratorKey];

        if (!propsDecorators) {
          return undefined;
        }

        return Object.entries(propsDecorators).reduce(
          (previousValue, [key, value]) => {
            if (value[0].ngMetadataName === 'Input') {
              const propertyName = value[0].bindingPropertyName ?? key;
              return {
                ...previousValue,
                inputs: [...previousValue.inputs, propertyName],
              };
            }
            if (value[0].ngMetadataName === 'Output') {
              const propertyName = value[0].bindingPropertyName ?? key;
              return {
                ...previousValue,
                outputs: [...previousValue.outputs, propertyName],
              };
            }
            return previousValue;
          },
          {
            inputs: [],
            outputs: [],
          }
        );
      };
      console.log(getComponentIOPropertiesName(storyObj.component));

if it suits you. We will have to define what to do with it because I'm not sure I understand all your concerns 🙃

@Marklb
Copy link
Member

Marklb commented Nov 25, 2020

When I mentioned filtering, I was just meaning to only select the props that are an input or output, to avoid the NO_ERRORS_SCHEMA issue that you mentioned, but I think that the inputs/outputs have to be known either way to build the template.

That getComponentIOPropertiesName function would miss inputs in @Component and extended classes. So, in the following example, baseInp2, baseInp, and inp2 would be missed.

@Component({ selector: '' })
class BaseExample2 {
  @Input() baseInp2: string
}

@Component({ selector: '' })
class BaseExample extends BaseExample2 {
  @Input() baseInp1: string
}

@Component({
  selector: 'story-example',
  template: ``,
  inputs: [ 'inp2' ]
})
class StoryExample extends BaseExample {
  @Input() inp1: string
  @Input('reInp3') inp3: string
}

I do see them in other props on the object though, so it should be able to work with some adjustments, since that is probably what they are doing to initialize the factory in JIT also. Looks like the inputs/outputs from @Component are in __annotations__.inputs/__annotations__.outputs and the inputs/outputs from extended classes by iterating each extended class on the prototype.

@ThibaudAV
Copy link
Contributor Author

@Marklb Do you think this is the proper approach to use?
If yes I can add a commit with tests for all cases. To find all inputs/outputs

@Marklb
Copy link
Member

Marklb commented Nov 25, 2020

@Marklb Do you think this is the proper approach to use?
If yes I can add a commit with tests for all cases. To find all inputs/outputs

Yes. At first I was leaning toward just using the factory, but now I am liking your approach more. One reason is because it could potentially be used by other features that aren't running in Angular, like #12438

@ThibaudAV
Copy link
Contributor Author

ThibaudAV commented Nov 26, 2020

@Marklb I've added 2 commits on the last discussed subjects

I have also added more "complete" tests. In order to be able to be more safe about the link between the component and the props.

Tell me what you think? Do you think this pr is a good thing ? and that we should finish it and test it well, to merge it in next ?

🤔 I wonder if it wouldn't be better to add an useBetaRendering option in the story and keep both working. This would allow real life testing and unblocking of some issue ?

Don't hesitate if you want to add code or modify the existing one.

@gaetanmaisse gaetanmaisse changed the title 🎨 Rewrite rendering feature of angular client Angular: Refactor preview renderer Nov 28, 2020

import { getComponentInputsOutputs } from './NgComponentAnalyzer';

describe('getComponentInputsOutputs', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome test suite, it will make us much more confident about everything related to component properties detection 💪🏻

app/angular/src/client/preview/angular/app.token.ts Outdated Show resolved Hide resolved
@Marklb
Copy link
Member

Marklb commented Dec 1, 2020

@Marklb do you have in mind other things that should land in this PR?

@gaetanmaisse Anything else I can think of at the moment would be new features made possible/easier from this. So, they would be separate PR's.

@shilman
Copy link
Member

shilman commented Dec 2, 2020

@Marklb @gaetanmaisse sounds like this PR is good to go then? @ThibaudAV Can you get the tests to pass?

@@ -32,4 +32,6 @@ export interface StoryFnAngularReturnType {
moduleMetadata?: NgModuleMetadata;
template?: string;
styles?: string[];
/** Uses legacy angular rendering engine that use dynamic component */
useLegacyRendering?: boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about making this a framework option. User would write the following in .storybook/main.js:

module.exports = {
  angularOptions: {
    legacyRendering: true,
  }
}

Another option would be to use story parameters if we want to control this on a per story basis. To configure globally, .storybook/preview.js:

export const parameters = {
  angularLegacyRendering: true,
}

Both of these would be more "storybook-like" than adding a new field to the story function return type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of the framework option, because that projects rendering would be consistent, instead of potentially introducing bugs from the previous renderer not getting cleaned up.

As long as the renderer's clean themselves up when switching, then users with a large project may have a reason to migrate a few stories at a time. So, a parameter may be the better choice to avoid users not being able to start migrating, because of a bug or situation we didn't consider.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small suggestion about configuration via framework options or parameters. See comment line @ThibaudAV @gaetanmaisse

@ThibaudAV ThibaudAV force-pushed the new-angular-client branch 3 times, most recently from 7add6e7 to 9a24909 Compare December 2, 2020 10:12
@ThibaudAV
Copy link
Contributor Author

ThibaudAV commented Dec 2, 2020

@shilman I added it as a parameters :
30d3a19#diff-2301c93915662308821a28d0c92f96d5bb1253c16872c6f3aeac982087ddc006R31
I hope this is the good way to do it ?

I also corrected tests

A `useLegacyRendering` flag allows to use old rendering engine
Configure app/angular package as jest projects
Allow to use jest-preset-angular for app/angular tests
The test allows to verify the function by using angular engine
…rties

- allow story with component to override local properties
- handles Input with `bindingPropertyName`
@shilman shilman modified the milestones: 6.2 docs, 6.2 maintenance Dec 3, 2020
@shilman shilman changed the title Angular: Refactor preview renderer Angular: Overhaul preview renderer Dec 3, 2020
@kroeder
Copy link
Member

kroeder commented Dec 29, 2020

Impressive PR, great job! 💪

@shilman
Copy link
Member

shilman commented Jan 4, 2021

cc @mandarini

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
angular maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants