Angular: Updates to Angular argsToTemplate

[JSMike]

Update default functionality of argsToTemplate to align with autodocs data binding. Use existing function that's used by autodocs to replace variable names with current values.
Add toggle to use variable names instead of values.
Add option to sort keys alphabetically.
Update order of bindings to group data bindings before event listener bindings.
Add option to define default values in order to remove unecessary/redundant bindings from being shown.

Closes #24557

What I did

Update functionality of argsToTemplate to be closer to existing non-template solution which shows the current value of bindings. Add quality of life features.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

• stories
• unit tests
• integration tests
• end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Run angular sandbox for template, e.g. yarn task --task dev --template angular-cli/default-ts --prod --start-from=compile
Open Storybook in your browser
Navigate to Button/Docs
Select 'Show code' on the Primary canvas and note structure.
Open editor to sandbox/angular-cli-default-ts/src/stories/button.stories.ts
In the Primary story add:

  render: (args) => ({
    props: args,
    template: `<storybook-button ${argsToTemplate(args)}></storybook-button>`
  })

Select 'show code' again on the Primary story. The code snippet should match.

Also added sort, useVariableNames, and defaultValues options to argsToTemplate. Can manually test those features.

Documentation

• Add or update documentation reflecting your changes
• If you are deprecating/removing a feature, make sure to update
  MIGRATION.MD

Checklist for Maintainers

• When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

• Make sure this PR contains one of the labels below:

  Available labels

  • bug: Internal changes that fixes incorrect behavior.
  • maintenance: User-facing maintenance tasks.
  • dependencies: Upgrading (sometimes downgrading) dependencies.
  • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
  • cleanup: Minor cleanup style change. Will not show up in release changelog.
  • documentation: Documentation only changes. Will not show up in release changelog.
  • feature request: Introducing a new feature.
  • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
  • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

[greptile-apps]

_{3 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings}

[greptile-apps]

style: This condition might be too complex. Consider extracting it into a separate function for better readability

[greptile-apps]

style: Consider using a switch statement or separate functions for each case to improve readability

[nx-cloud]

☁️ Nx Cloud Report

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

📂 See all runs for this CI Pipeline Execution

---

✅ Successfully ran 1 target

• nx run-many -t build --parallel=3

---

Sent with 💌 from NxCloud.

[JSMike]

I would like feedback on some of the features added before I add additional tests/documentation.

[JSMike]

Maybe also related to: #27531

[Marklb]

I haven't got a chance to thoroughly go through this, but I took a quick look for anything obvious that I could see. There may be something else, if I run the code, which I will try to do, but only one thing stood out.

For Update order of bindings to group data bindings before event listener bindings., I don't know if that can be done, without access to the argTypes or component. Unless I am just missing something, it looks like you are determining outputs based on if they are a function. In most scenarios that is correct, but one sort of common situation that would be wrong is a component that takes in a filtering function as input.

[Marklb]

The following is not great and has some side-effects, but it is something I have been using in one of my projects. I am only using for 6 component's stories, so it hasn't been used by much, but just throwing out the idea.

My function takes advantage of the render function being able to use hooks.

Benefits of my function:

• Don't need to deal with inputs.
• Can configure globally, per stories file, per story or per function.

Problems with my function:

• If you don't call the function in the render function then the context may not be available or done being populated by decorators.
• Expects argTypes to be populated. Probably not difficult to replace with parsing component metadata.
• By removing the need to pass any inputs, it forces configuration to exclude anything that shouldn't be used from args, which may be frustrating if there are a lot of args.

I don't like how my solution removes the simplicity of calling the function from anywhere, by forcing it to be called from a place that can access the hooks. That is one reason I keep being undecided on making the function smarter or staying simple.

I also was going to look at replacing my function's use of argTypes with the way Storybook differentiates inputs/outputs, internally, by parsing the metadata of the component. The component should be easily accessed from the context, unless your story isn't setting a component for the stories. I just didn't get around to trying it that way.

import { AngularRenderer } from '@storybook/angular'
import { useStoryContext } from '@storybook/preview-api'

function removeDuplicates(arr: string[]) {
  const seen: { [k: string]: boolean } = {}
  return arr.filter(item => {
    if (!seen[item]) {
      seen[item] = true
      return true
    }
    return false
  })
}

/**
 * Options to set in parameters.
 */
export interface ArgsTplOptions {
  /**
   * Properties to always bind to the template.
   */
  alwaysBind?: string[]
  /**
   * Properties to exclude from binding to the template.
   */
  exclude?: string[]
}

/**
 * This is an attempt at simplifying the use of auto-generated args in stories
 * defined with `template`.
 *
 * @experimental
 */
export function argsToTpl(options?: ArgsTplOptions) {
  const context = useStoryContext<AngularRenderer>()

  const exclude = [
    ...(context?.parameters?.argsToTplOptions?.exclude || []),
    ...(options?.exclude || []),
  ]

  const alwaysBind = context?.parameters?.argsToTplOptions?.alwaysBind || []

  const props = removeDuplicates([
    ...alwaysBind,
    ...Object.keys(context.args),
  ])

  const parts = props
    .filter(k => exclude.indexOf(k) === -1)
    .map(k => {
      // Outputs
      if (
        context.argTypes[k]?.hasOwnProperty('action') &&
        (context.args.hasOwnProperty(k) || alwaysBind.indexOf(k) !== -1)
      ) {
        return `(${k})="${k}($event)"`
      }

      // Inputs
      if (
        (context.args.hasOwnProperty(k) || alwaysBind.indexOf(k) !== -1)
      ) {
        return `[${k}]="${k}"`
      }
    })

  // TODO: Toggle page breaks to print inline or aligned vertically.
  return parts.length > 0 ? parts.join(' ') : ''
}

[JSMike]

I haven't got a chance to thoroughly go through this, but I took a quick look for anything obvious that I could see. There may be something else, if I run the code, which I will try to do, but only one thing stood out.

For Update order of bindings to group data bindings before event listener bindings., I don't know if that can be done, without access to the argTypes or component. Unless I am just missing something, it looks like you are determining outputs based on if they are a function. In most scenarios that is correct, but one sort of common situation that would be wrong is a component that takes in a filtering function as input.

Per the existing logic of argsToTemplate all functions are considered to be event listeners, which I agree isn't the best way to do it, but just carried that pattern forward. It would be much better to have the context of metadata/argtypes. I wouldn't mind removing the sort from this, I added a couple of the features for quality of life but they're not as needed. I believe the current order for args is the order of keys in argTypes, so if users want to sort their event emitters last they could define them last in argTypes.

The main goal for this PR is to align databinding to current value instead of the variable name so that users could see the value that was being passed to the component and so working code examples could be copy/pasted from the canvas code snippet, similar to autodocs without a manual template.

[JSMike]

My function takes advantage of the render function being able to use hooks.

Thanks for showing that example, I wasn't aware of being able to generate a context, I could see this being much simpler with the context. Since the function is literally called argsToTemplate I think it would be safe to call out that it's meant to be used within a template of a render function. I understand your concern with it not working outside of a render function, but that could be called out in documentation if that would be the desired effect (although then potentially a breaking change if the function was originally intended to be called from any context).

[Marklb]

I believe the current order for args is the order of keys in argTypes, so if users want to sort their event emitters last they could define them last in argTypes.

Yeah, I don't know what would be best, because I see the benefit, but also the probably minor problems.

There is another side-effect that I just remembered, but I don't remember where I explained it before. I will link it if I find it. Now, that I am thinking about it more, I think that it is the reason I stopped trying to improve that function I posted above.

Replacing the variable with the value affects rendering. In most components, you probably won't notice it, but every time an args value changes, the story fully rerenders. It is most noticeable on components that have an :enter animation or async action in ngOnInit, because they will rerun on each args change. The reason is that you are altering the template and the template changing is something that triggers a full rerender of the story.

The reason is that normally, Storybook actually uses a template that is similar to what the current argsToTemplate generates. code/frameworks/angular/src/client/docs/sourceDecorator.ts then generates the template again, if a template isn't provided, and that template is the one with replaced variables that get's sent to the displayed code snippet.

I wanted to try and come up with a solution that avoided this, but I didn't have a good idea. I will look for my previous discussion and see if I had any idea that I am forgetting.

[JSMike]

Replacing the variable with the value affects rendering. In most components, you probably won't notice it, but every time an args value changes, the story fully rerenders.

ok, I did notice additional renders, but in most/all cases the rerender didn't have a negative impact for our components. Maybe I can explore an enhancement to sourceDecorator that would enable custom template strings to work more like autodocs.

Or: (just had this thought) a different/similar argsToTemplate function that would update the context of Source instead of the story canvas.