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: Fix Cannot read property 'selector' of undefined #15874

Merged
merged 2 commits into from Aug 23, 2021

Conversation

saulodias
Copy link

@saulodias saulodias commented Aug 18, 2021

Issue: #14828

What I did

This fixes the issue 14828.

 Cannot read property 'selector' of undefined

This issue happens when trying to import an Angular component which has been already compiled. The Issue is explained here by @KondakovArtem.

Here is a reproduction of the issue. All the code except for the last commit, where I created the my-lib.stories.ts file has been automatically generated with the following commands, using the @angular/cli@11.2.14.

ng new storybook-i14828 --create-application=false
cd storybook-i14828
ng generate library my-lib
npx sb init

How to test

  • Is this testable with Jest or Chromatic screenshots?

    • The previous Jest test already safisfies the condition. I made sure the method will still return undefined, which was already a valid tested return type, if it doesn't find any decorators.
  • Does this need a new example in the kitchen sink apps?

  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Aug 18, 2021

Nx Cloud Report

CI ran the following commands for commit 1508807. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@saulodias saulodias force-pushed the fix-issue-14828 branch 2 times, most recently from ca0acda to 5bcfdae Compare August 18, 2021 20:54
@saulodias saulodias changed the title Fix issue 14828 Fix issue 14828 🐛 Aug 18, 2021
@saulodias saulodias changed the title Fix issue 14828 🐛 Fix issue 14828 Aug 18, 2021
@saulodias saulodias changed the title Fix issue 14828 Fix issue #14828 Aug 18, 2021
@saulodias saulodias changed the title Fix issue #14828 Angular: Fix Cannot read property 'selector' of undefined #14828 Aug 18, 2021
@shilman shilman requested a review from ThibaudAV August 19, 2021 17:46
@shilman shilman changed the title Angular: Fix Cannot read property 'selector' of undefined #14828 Angular: Fix Cannot read property 'selector' of undefined Aug 19, 2021
yarn.lock Outdated Show resolved Hide resolved
@@ -136,5 +136,14 @@ export const getComponentDecoratorMetadata = (component: any): Component | undef
? Reflect.getOwnPropertyDescriptor(component, decoratorKey).value
: component[decoratorKey];

return (decorators || []).find((d) => d instanceof Component);
if (!decorators) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add some test on this file ? https://github.com/storybookjs/storybook/blob/5bcfdae1b6f5d7849a0a48b58a306410977e0cf5/app/angular/src/client/preview/angular-beta/utils/NgComponentAnalyzer.test.ts

To run simple test yarn jest NgComponentAnalyzer.test.ts or with watch mode yarn jest NgComponentAnalyzer.test.ts --watch
don't hesitate to ask me for help if needed ;)

Copy link
Author

@saulodias saulodias Aug 20, 2021

Choose a reason for hiding this comment

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

Hey, I tried my best to create a test at least for the "regular" case. That is, when you are trying to use a component which is not from an Angular library bundle. That test didn't exist indeed. However, once you compile the components into a library, everything becomes very "plain" (objects and functions) and we cannot take advantage of the Angular native types that much to create clean tests, or at least I could not come up with a way to do that in a clean way. So what here is some evidence, if you will, just for the sake of traceability in regards to the expected behavior:

image

Instead of:
image

Copy link
Contributor

@ThibaudAV ThibaudAV Aug 20, 2021

Choose a reason for hiding this comment

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

👍
Top these evidence.
If you want you can add them here :

https://github.com/storybookjs/storybook/tree/1508807079ecb35a34f8f05c7d60a179b770504a/examples/angular-cli/src/stories/basics
As you can see there are others. It's also a way to test the application when unit testing is not efficient / possible
The examples try to be autonomous and represent a basic use case in "real life"

Copy link
Author

Choose a reason for hiding this comment

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

I'm working on a minimal Angular Library setup for that angular-cli workspace. I'll let you know when I'm done and we can discuss some minor details. But we'll cross that bridge when we get to it.

@ThibaudAV
Copy link
Contributor

❤️ thanks for contribution :) LGTM (Just the test, if possible)

@shilman shilman merged commit 3ad9c4e into storybookjs:next Aug 23, 2021
@shilman shilman added this to the 6.4 PRs milestone Aug 23, 2021
@saulodias saulodias deleted the fix-issue-14828 branch August 23, 2021 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants