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

Using storyStoreV7 with Angular returns "undefined" using componentWrapperDecorator #17330

Closed
spaceribs opened this issue Jan 25, 2022 · 9 comments

Comments

@spaceribs
Copy link
Contributor

spaceribs commented Jan 25, 2022

Describe the bug
When enabling storyStoreV7 with Angular 13, using componentWrapperDecorator returns undefined within the story rendering.

To Reproduce
See: https://github.com/spaceribs/storybook-angular-test

All stories on that repo should be using componentWrapperDecorator and are displaying with a red background and the text undefined.

System
System:
OS: macOS 12.0.1
CPU: (10) x64 Apple M1 Pro
Binaries:
Node: 12.22.8 - ~/.nvm/versions/node/v12.22.8/bin/node
npm: 6.14.15 - ~/.nvm/versions/node/v12.22.8/bin/npm
Browsers:
Chrome: 97.0.4692.99
Firefox: 96.0.1
Safari: 15.1
npmPackages:
@storybook/addon-a11y: ^6.4.10 => 6.4.10
@storybook/addon-actions: 6.4.14 => 6.4.14
@storybook/addon-controls: 6.4.14 => 6.4.14
@storybook/addon-docs: 6.4.14 => 6.4.14
@storybook/addon-essentials: 6.4.14 => 6.4.14
@storybook/addon-interactions: 6.4.14 => 6.4.14
@storybook/addon-viewport: 6.4.14 => 6.4.14
@storybook/addons: 6.4.14 => 6.4.14
@storybook/angular: 6.4.14 => 6.4.14
@storybook/builder-webpack5: 6.4.14 => 6.4.14
@storybook/html: 6.4.14 => 6.4.14
@storybook/manager-webpack5: 6.4.14 => 6.4.14
@storybook/theming: 6.4.14 => 6.4.14

Additional context
None

@GabrielAPineiro
Copy link

Is this bug fixed? Still happening in @storybook 6.5.9

@spaceribs
Copy link
Contributor Author

@GabrielAPineiro I've updated the example to 6.5.9 and can confirm the same issues exist.

@Marklb
Copy link
Member

Marklb commented Jul 19, 2022

I need to go through the StoryStore changes to figure out how the fix may need to be done, because I haven't caught myself up on that change yet. I think I see why this is happening though.

The following is what I found, if someone more familiar with the StoryStore knows how this should be changed.

In the angular app the decorateStory function called a prepareMain function that created a template for the component. https://github.com/storybookjs/storybook/blob/next/app/angular/src/client/preview/decorateStory.ts#L23

The StoryStore doesn't seem to be using the decorateStory function defined in the angular app and instead just seems to be using a common decorateStory function. It was getting called by defining it here: https://github.com/storybookjs/storybook/blob/next/lib/store/src/decorators.ts#L20

To just see if I could get this issue's repro to work, I hard-coded a change to the new decorateStory, which is not app specific. The following is how I patched my node_modules:

Without my patch:

// node_modules\@storybook\store\dist\esm\decorators.js
...
export function decorateStory(storyFn, decorator, bindWithContext) {
  // Bind the partially decorated storyFn so that when it is called it always knows about the story context,
  // no matter what it is passed directly. This is because we cannot guarantee a decorator will
  // pass the context down to the next decorated story in the chain.
  var boundStoryFunction = bindWithContext(storyFn);
  return function (context) {
    return decorator(boundStoryFunction, context);
  };
}
...

With my patch:

// node_modules\@storybook\store\dist\esm\decorators.js
...
const ComputesTemplateFromComponent_1 = require("@storybook/angular/dist/ts3.9/client/preview/angular-beta/ComputesTemplateFromComponent");
const prepareMain = (story, context) => {
  var _a;
  let { template } = story;
  const component = (_a = story.component) !== null && _a !== void 0 ? _a : context.component;
  const userDefinedTemplate = !hasNoTemplate(template);
  if (!userDefinedTemplate && component) {
      template = ComputesTemplateFromComponent_1.computesTemplateFromComponent(component, story.props, '');
  }
  return Object.assign(Object.assign({}, story), (template ? { template, userDefinedTemplate } : {}));
};
function hasNoTemplate(template) {
  return template === null || template === undefined;
}

export function decorateStory(storyFn, decorator, bindWithContext) {
  // Bind the partially decorated storyFn so that when it is called it always knows about the story context,
  // no matter what it is passed directly. This is because we cannot guarantee a decorator will
  // pass the context down to the next decorated story in the chain.
  var boundStoryFunction = bindWithContext(storyFn);
  return function (context) {
    return prepareMain(decorator(boundStoryFunction, context), context);
  };
}
...

Even though that patch works, at least for the simple repro provided, it isn't a good fix, because it is importing a framework specific call in common code. Does the StoryStore have a place for frameworks to hook into that step, like they used to with the start function, or an alternative way they should be doing this? I will look into it some more when I get a chance, if not.

@tmeasday
Copy link
Member

Great analysis @Marklb and sorry for the delayed response. You are absolutely right and the answer is pretty simple:

Does the StoryStore have a place for frameworks to hook into that step, like they used to with the start function, or an alternative way they should be doing this?

Yes, they should export an annotation applyDecorators from their preview "config" file. This is a simple bug (🤦 ) -- it is exported as the wrong thing:

export { decorateStory } from './decorateStory';

Compare to the vue renderer:

export { decorateStory as applyDecorators } from './decorateStory';

@tmeasday
Copy link
Member

Would you like to submit a PR fixing it?

@spaceribs
Copy link
Contributor Author

Would you like to submit a PR fixing it?

Done, please review

@tmeasday tmeasday reopened this Sep 16, 2022
@alexciesielski
Copy link

I'm having the same problem in 7.0.0-alpha.33

@spaceribs
Copy link
Contributor Author

I'm having the same problem in 7.0.0-alpha.33

I don't believe these changes have been released yet, please wait until 7.0.0-alpha.34 at least

@spaceribs
Copy link
Contributor Author

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

6 participants