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

[Bug] Angular Components #2647

Closed
amcdnl opened this issue Jan 4, 2018 · 11 comments
Closed

[Bug] Angular Components #2647

amcdnl opened this issue Jan 4, 2018 · 11 comments

Comments

@amcdnl
Copy link
Member

amcdnl commented Jan 4, 2018

I have 3 different bugs I've identified in this repo: https://github.com/amcdnl/angular-storybook-issue

Basic

In the basic scenario, I import my app component and nothing renders.

Basic 2

In the basic 2 scenario, I use a component directly that has a nested module dependency and its not resolving that module. It throws the following errors:

'app-test' is not a known element:
1. If 'app-test' is an Angular component, then verify that it is part of this module.
2. If 'app-test' is a Web Component then add 'CUSTOM_ELEMENTS_SCHEMA' to the '@NgModule.schemas' of this component to suppress this message. ("<div>
  Test?
  [ERROR ->]<app-test></app-test>
  <nav aria-label="Page navigation">
    <ul class="pagination">
"): ng:///NewModule/NewComponent.html@2:2 ; Zone: <root> ; Task: Promise.then ; Value: Error: Template parse errors:
'app-test' is not a known element:

CSS

The boostrap.css file included config.js is never injected.

@ralzinov
Copy link
Contributor

ralzinov commented Jan 4, 2018

Hello thank you for feedback, i have looked on your example, and at first story you trying to bootstrap app.component into another module, but i think it is not valid case (tell me if i wrong, i have checked and got exactly the same error with pure angular)
I think you should export component from module if you want it to render. Second is that "app-root" selector is actually occupied by storybook itself, (so that definitely should be fixed). With these changes fixed, the first example is working.

At second example you are trying to render component, that exported from nested module directly. And at that case i've got such error too with pure angular, example.

@amcdnl
Copy link
Member Author

amcdnl commented Jan 5, 2018

Any ideas on the styles? The others I can work around.

@alterx
Copy link
Member

alterx commented Jan 5, 2018

Yeah, the explanation for this behavior is that we create an inner module that contains our main component (this component dynamically renders each story). Since the nested TestingModule module can only be used by the module that's importing it (AppModule), the component is not available for our own module. Doing something like this should work for issue # 2:

@NgModule({
  declarations: [
    AppComponent
  ],
  imports: [
    BrowserModule,
    TestModule
  ],
  providers: [],
  bootstrap: [AppComponent],
  exports: [
    BrowserModule,
    TestModule
  ]
})
export class AppModule { }

EDIT: For the first bug, it's true that we should change the name of our root node but that's not the source of the bug. Actually the same explanation applies: app-test.component.ts it's using a component that's available inside the AppModule but isn't available in our own storybook module. Exporting the component the same way we did with the modules fixes the issue.

I'm also issuing a PR to change the name of our root node (it'll probably cause troubles eventually)

@igor-dv
Copy link
Member

igor-dv commented Jan 5, 2018

@amcdnl , regarding the css:

Css in this case used in two different context:

  1. like an import as you did - for this we use style-loader
  2. like a styleUrl in component - for this we need raw-loader

So we have a collision between css rules.

What can you do: Use this feature for the boostrap.css. Or even this.

Or add an exclusion for the bootstrap.css in the custom webpack.config (there is already one for components):

  const cssRule2 = config.module.rules.find(rule => rule.test && rule.test.toString() === '/\\.(html|css)$/');
  if (cssRule2) {
    cssRule2.exclude = /bootstrap\.css$/;
  }

@amcdnl
Copy link
Member Author

amcdnl commented Jan 5, 2018

I was just showing that 2 different ways of doing it. Neither worked for me.

@Hypnosphi
Copy link
Member

Fix released as 3.3.4

@igor-dv
Copy link
Member

igor-dv commented Jan 7, 2018

@amcdnl , did you manage to make css working?

@alterx
Copy link
Member

alterx commented Jan 7, 2018

Not sure we should be closing this one. Issues 1 and 2 have a workaround and an explanation so I think we're good with those (#2657 is not directly related).

It looks like issue # 3 is still happening tho

@Hypnosphi Hypnosphi reopened this Jan 8, 2018
@Hypnosphi
Copy link
Member

Hypnosphi commented Jan 8, 2018

Oh sorry. It just stated "Issue: #2647"
Now I see that this is an umbrella issue for 3 different bugs

@amcdnl
Copy link
Member Author

amcdnl commented Jan 8, 2018

The following config worked me around the CSS issue.

const genDefaultConfig = require('@storybook/angular/dist/server/config/defaults/webpack.config.js');

module.exports = (baseConfig, env) => {
    const config = genDefaultConfig(baseConfig, env);

    const cssRule = config.module.rules.find(rule => rule.test && rule.test.toString() === '/\\.css$/');
    if (cssRule) {
        cssRule.exclude = /\.component\.css$/;
    }

    const cssRule2 = config.module.rules.find(rule => rule.test && rule.test.toString() === '/\\.(html|css)$/');
    if (cssRule2) {
        cssRule2.exclude = /bootstrap\.css$/;
    }

    config.module.rules.unshift({
        test: /\.component.scss$/,
        loaders: ['raw-loader', 'sass-loader']
    });

    config.module.rules.unshift({
        test: /\.component.css$/,
        loaders: ['raw-loader', 'css-loader']
    });

    config.module.rules.unshift({
        test: /\.scss$/,
        exclude: /\.component.scss$/,
        loaders: ['style-loader', 'css-loader', 'sass-loader']
    });

    return config;
};

@alterx
Copy link
Member

alterx commented Jan 11, 2018

Will close this one since there's a workaround for issue # 3 and, # 1 and # 2 work as expected. Also, there's related work happening in #2688 🎉

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