[Angular] [bug] Automatic component declaration for Angular not working with forRoot

[pascaliske]

Describe the bug
One of the last Storybook releases introduced a new behavior for NgModules where Storybook only declares components dynamically if they were not declared already inside of an imported NgModule (see #6666 - Thanks to @MaximSagan for his awesome work!).

This new behavior also introduces an error with NgModules imported by a static forRoot method which is a really common pattern for Angular apps:

TypeError: Cannot read property 'value' of undefined

I already traced the error down to the line 60 in angular helpers. I would suggest to check the importItem for an existing key ngModule to identify forRoot-imports.

Click to expand diff

  const extractNgModuleMetadata = (importItem: any): NgModule => {
+ const target = importItem && importItem.ngModule ? importItem.ngModule : importItem;
  const decoratorKey = '__annotations__';
  const decorators: any[] =
    Reflect && Reflect.getOwnPropertyDescriptor
-     ? Reflect.getOwnPropertyDescriptor(importItem, decoratorKey).value
-     : importItem[decoratorKey];
+     ? Reflect.getOwnPropertyDescriptor(target, decoratorKey).value
+     : target[decoratorKey];

    if (!decorators || decorators.length === 0) {
      return null;
    }

    const ngModuleDecorator: NgModule | undefined = decorators.find(
      decorator => decorator instanceof NgModule
    );
    if (!ngModuleDecorator) {
      return null;
    }
    return ngModuleDecorator;
  };

I can provide a PR with this fix if you're fine with this change.

To Reproduce
Repo: https://github.com/pascaliske/form-elements
Branch: feature/v8
Problematic line: https://github.com/pascaliske/form-elements/blob/feature/v8/.storybook/config.ts#L24
Steps to reproduce the behavior:

1. Clone repo at branch feature/v8
2. Start storybook with yarn run build && yarn run storybook
3. See error in almost every story

Expected behavior
Storybook can extract Metadata from imported NgModules both with or without forRoot.

Screenshots
n/a

Code snippets
n/a

System:

• OS: MacOS 10.14.5
• Device: Macbook Pro 2019
• Browser: chrome
• Framework: angular
• Addons: notes, knobs, actions, links
• Version: v5.1.8

Additional context
n/a

[methodus]

Maybe my issue (#7157) is related to this. Any work-a-rounds?

[pascaliske]

@methodus Yes, your issue describes the same bug.

I have a workaround (provided inside the collapsed diff above) which requires to edit the storybook source code inside node_modules but im willing to extract a PR from this issue changing the storybook code for a future release.

Until the issue is fixed I would suggest to create a little script that applies the above diff and put that script in your npm / yarn postinstall script.

[methodus]

Do you mean this commit? pascaliske/form-elements@3f00b3c

[pascaliske]

@methodus yes but inside the storybook-patch.js you need to change the search and replace strings for the above diff. But beware: changing code inside node_modules can lead to unexpected errors. 😉

storybook-patch.js

#!/usr/bin/env node
const { readFile, writeFile } = require('fs')
const { join, dirname } = require('path')
const { promisify } = require('util')
const chalk = require('chalk')

/* tslint:disable:max-line-length */
const SEARCH = `
    var decoratorKey = '__annotations__';
    var decorators = Reflect && Reflect.getOwnPropertyDescriptor
        ? Reflect.getOwnPropertyDescriptor(importItem, decoratorKey).value
        : importItem[decoratorKey];
`
const REPLACE = `
    var target = importItem && importItem.ngModule ? importItem.ngModule : importItem;
    var decoratorKey = '__annotations__';
    var decorators = Reflect && Reflect.getOwnPropertyDescriptor
        ? Reflect.getOwnPropertyDescriptor(target, decoratorKey).value
        : target[decoratorKey];
`
/* tslint:enable:max-line-length */

async function execute() {
    const path = dirname(require.resolve('@storybook/angular'))
    const file = join(path, 'preview', 'angular', 'helpers.js')
    const contents = await promisify(readFile)(file, 'utf8')

    // only replace the exact search pattern
    if (!contents.includes(SEARCH)) {
        console.warn(
            chalk.yellow(
                // tslint:disable-next-line
                '@storybook/angular was not patched as their source code changed or the patch is already applied.',
            ),
        )
    }

    await promisify(writeFile)(file, contents.replace(SEARCH, REPLACE), 'utf8')
}

execute().catch(e => console.error(e.toString()))

[cormickjbrowne]

@pascaliske I ran your script and I can see that this line...

var target = importItem && importItem.ngModule ? importItem.ngModule : importItem;

...was added in
node_modules\@storybook\angular\dist\client\preview\angular\helpers.js

but I'm still seeing the same error in storybook. The importItem that it's trying to load looks like this:

/**
 * @fileoverview added by tsickle
 * @suppress {checkTypes} checked by tsc
 */
var HighchartsChartModule = /** @class */ (function () {
    function HighchartsChartModule() {
    }
    HighchartsChartModule.decorators = [
        { type: NgModule, args: [{
                    declarations: [HighchartsChartComponent],
                    exports: [HighchartsChartComponent]
                },] }
    ];
    /** @nocollapse */
    HighchartsChartModule.ctorParameters = function () { return []; };
    return HighchartsChartModule;
}());

Any thoughts?

[pascaliske]

EDIT: @cormickjbrowne I actually figured out why it does not work for you:
It seems the feature of #6666 only works for imports from source files (*.ts). The error will appear anyway for imports of compiled Angular (AOT) code (*.js) because the property __annotations__ will be stripped out by the Angular AOT compiler. I will keep digging the Angular source code - hopefully I'll find a way to make the feature work with AOT compiled code. 🙂

@cormickjbrowne How did you import your module in Storybook? Is the output above from logging the importItem? If you log out the target variable it should always contain the actual NgModule which is required by storybook:

My fix above works for the cases where the forRoot convention of Angular is used. The forRoot returns an object containing the NgModule with providers instead of providing the NgModule directly which will be considered by the target variable.

[pascaliske]

For my own Angular library I can confirm that the automatic component declaration works together with compiled files (built with ng-packagr) if I build the lib with the Angular compiler flag "annotateForClosureCompiler": false, turned off. This compiles Angulars decorators using tslibs __decorate function which injects the __annotations__ metadata correctly.

[pascaliske]

Besides the problem with AOT-compiled code I would suggest to implement the following change which fixes the actual error from #7157 and my error with the forRoot use case:

Click to expand diff

  const extractNgModuleMetadata = (importItem: any): NgModule => {
+   const target = importItem && importItem.ngModule ? importItem.ngModule : importItem;
    const decoratorKey = '__annotations__';
    const decorators: any[] =
-     Reflect && Reflect.getOwnPropertyDescriptor
-       ? Reflect.getOwnPropertyDescriptor(importItem, decoratorKey).value
-       : importItem[decoratorKey];
+     Reflect && Reflect.getOwnPropertyDescriptor && Reflect.getOwnPropertyDescriptor(target, decoratorKey)
+       ? Reflect.getOwnPropertyDescriptor(target, decoratorKey).value
+       : target[decoratorKey];

    if (!decorators || decorators.length === 0) {
      return null;
    }

    const ngModuleDecorator: NgModule | undefined = decorators.find(
      decorator => decorator instanceof NgModule
    );
    if (!ngModuleDecorator) {
      return null;
    }
    return ngModuleDecorator;
  };

[stale]

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!

[pascaliske]

It would be awesome if we can keep this open. I just provided a PR (#7224) to fix this error hopefully we can merge it. 😊

[shilman]

Ooh-la-la!! I just released https://github.com/storybookjs/storybook/releases/tag/v5.2.0-beta.9 containing PR #7224 that references this issue. Upgrade today to try it out!

You can find this prerelease on the @next NPM tag.

Closing this issue. Please re-open if you think there's still more to do.

[pascaliske]

The bugfix from the PR works as expected. Thanks! 🙂