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.

Sign up for GitHub

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
Merged

Angular: Fix Cannot read property 'selector' of undefined #15874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
235f498
add test for getComponentDecoratorMetadata
saulodias Aug 20, 2021
1508807
fix issue #14828
saulodias Aug 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion app/angular/src/client/preview/angular-beta/utils/NgComponentAnalyzer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,12 @@ import {
import { TestBed } from '@angular/core/testing';
import { BrowserDynamicTestingModule } from '@angular/platform-browser-dynamic/testing';

import { getComponentInputsOutputs, isComponent, isDeclarable } from './NgComponentAnalyzer';
import {
getComponentInputsOutputs,
isComponent,
isDeclarable,
getComponentDecoratorMetadata,
} from './NgComponentAnalyzer';

describe('getComponentInputsOutputs', () => {
it('should return empty if no I/O found', () => {
Expand Down Expand Up @@ -159,6 +164,15 @@ describe('isComponent', () => {
});
});

describe('getComponentDecoratorMetadata', () => {
it('should return Component with a Component', () => {
@Component({})
class FooComponent {}

expect(getComponentDecoratorMetadata(FooComponent)).toBeInstanceOf(Component);
});
});

function sortByPropName(
array: {
propName: string;
Expand Down
11 changes: 10 additions & 1 deletion app/angular/src/client/preview/angular-beta/utils/NgComponentAnalyzer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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.

return (
component.decorators &&
component.decorators[0] &&
component.decorators[0].args &&
component.decorators[0].args[0]
);
}

return decorators.find((d) => d instanceof Component);
};