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

Jump to bottom

Angular: Fix multiple calls of Input setter #17633

Merged
merged 1 commit into from Mar 28, 2022
Merged

Angular: Fix multiple calls of Input setter #17633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 38 additions & 0 deletions app/angular/src/client/preview/angular-beta/StorybookModule.test.ts
View file
Open in desktop
Expand Up @@ -14,6 +14,7 @@ describe('StorybookModule', () => {
template: `
<p id="input">{{ input }}</p>
<p id="inputBindingPropertyName">{{ localPropertyName }}</p>
<p id="setterCallNb">{{ setterCallNb }}</p>
<p id="localProperty">{{ localProperty }}</p>
<p id="localFunction">{{ localFunction() }}</p>
<p id="output" (click)="output.emit('outputEmitted')"></p>
Expand All @@ -27,6 +28,11 @@ describe('StorybookModule', () => {
@Input('inputBindingPropertyName')
public localPropertyName: string;

@Input()
public set setter(value: string) {
this.setterCallNb += 1;
}

@Output()
public output = new EventEmitter<string>();

Expand All @@ -36,6 +42,8 @@ describe('StorybookModule', () => {
public localProperty: string;

public localFunction = () => '';

public setterCallNb = 0;
}

it('should initialize inputs', async () => {
Expand Down Expand Up @@ -104,6 +112,7 @@ describe('StorybookModule', () => {
it('should change inputs if storyProps$ Subject emit', async () => {
const initialProps = {
input: 'input',
inputBindingPropertyName: '',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you are wondering why I am modifying the test here :

If a new entry was added to the list of props between the initialisation of the story and a lazy (emit on storyProps$) change of the input values
like : Initial { InputA : 'A' } new change : { InputA : 'A', InputB: 'B' }
The new Input was not added in the template like the first one <foo [InputA]="InputA" ></foo> because not new rendering a made only rxjs event with new props.
Before it worked because the property was added as a non Input Output property, overwriting the component property directly.
which was not good because angular could not detect the changes with the NgOnChanges for example

But anyway all this should not happen because a higher level mechanism detect if lazy rendering can be done. By comparing templates to see if there is a diff.

so in the test here all Input Output must be existing at init step

};
const storyProps$ = new BehaviorSubject<ICollection>(initialProps);

Expand Down Expand Up @@ -150,6 +159,7 @@ describe('StorybookModule', () => {
let expectedOutputValue;
let expectedOutputBindingValue;
const initialProps = {
input: '',
output: (value: string) => {
expectedOutputValue = value;
},
Expand Down Expand Up @@ -225,6 +235,34 @@ describe('StorybookModule', () => {
expect(fixture.nativeElement.querySelector('p').style.color).toEqual('black');
expect(fixture.nativeElement.querySelector('p#input').innerHTML).toEqual(newProps.input);
});

it('should call the Input() setter the right number of times', async () => {
const initialProps = {
setter: 'init',
};
const storyProps$ = new BehaviorSubject<ICollection>(initialProps);

const ngModule = getStorybookModuleMetadata(
{
storyFnAngular: { props: initialProps },
component: FooComponent,
targetSelector: 'my-selector',
},
storyProps$
);
const { fixture } = await configureTestingModule(ngModule);
fixture.detectChanges();

expect(fixture.nativeElement.querySelector('p#setterCallNb').innerHTML).toEqual('1');

const newProps = {
setter: 'new setter value',
};
storyProps$.next(newProps);
fixture.detectChanges();

expect(fixture.nativeElement.querySelector('p#setterCallNb').innerHTML).toEqual('2');
});
});

describe('with component without selector', () => {
Expand Down
32 changes: 3 additions & 29 deletions app/angular/src/client/preview/angular-beta/StorybookWrapperComponent.ts
View file
Open in desktop
Expand Up @@ -98,39 +98,13 @@ export const createStorybookWrapperComponent = (
this.storyComponentViewContainerRef.injector.get(ChangeDetectorRef).markForCheck();
this.changeDetectorRef.detectChanges();

// Once target component has been initialized, the storyProps$ observable keeps target component inputs up to date
// Once target component has been initialized, the storyProps$ observable keeps target component properties than are not Input|Output up to date
this.storyComponentPropsSubscription = this.storyProps$
.pipe(
skip(1),
map((props) => {
// removes component output in props
const outputsKeyToRemove = ngComponentInputsOutputs.outputs.map(
(o) => o.templateName
);
return Object.entries(props).reduce(
(prev, [key, value]) => ({
...prev,
...(!outputsKeyToRemove.includes(key) && {
[key]: value,
}),
}),
{} as ICollection
);
}),
map((props) => {
// In case a component uses an input with `bindingPropertyName` (ex: @Input('name'))
// find the value of the local propName in the component Inputs
// otherwise use the input key
return Object.entries(props).reduce((prev, [propKey, value]) => {
const input = ngComponentInputsOutputs.inputs.find(
(o) => o.templateName === propKey
);

return {
...prev,
...(input ? { [input.propName]: value } : { [propKey]: value }),
};
}, {} as ICollection);
const propsKeyToKeep = getNonInputsOutputsProps(ngComponentInputsOutputs, props);
return propsKeyToKeep.reduce((acc, p) => ({ ...acc, [p]: props[p] }), {});
})
)
.subscribe((props) => {
Expand Down