-
Notifications
You must be signed in to change notification settings - Fork 92
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
feat: support ngOnChanges with correct simpleChange object within rerender #366
Changes from 3 commits
e4acbbd
66ba0c7
4c5a255
d218907
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -123,14 +123,46 @@ export async function render<SutType, WrapperType = SutType>( | |
|
||
await renderFixture(componentProperties, componentInputs, componentOutputs); | ||
|
||
let renderedPropKeys = Object.keys(componentProperties); | ||
let renderedInputKeys = Object.keys(componentInputs); | ||
let renderedOutputKeys = Object.keys(componentOutputs); | ||
const rerender = async ( | ||
properties?: Pick<RenderTemplateOptions<SutType>, 'componentProperties' | 'componentInputs' | 'componentOutputs'>, | ||
properties?: Pick< | ||
RenderTemplateOptions<SutType>, | ||
'componentProperties' | 'componentInputs' | 'componentOutputs' | 'detectChangesOnRender' | ||
>, | ||
) => { | ||
await renderFixture( | ||
properties?.componentProperties ?? {}, | ||
properties?.componentInputs ?? {}, | ||
properties?.componentOutputs ?? {}, | ||
); | ||
const newComponentInputs = properties?.componentInputs ?? {}; | ||
for (const inputKey of renderedInputKeys) { | ||
if (!Object.prototype.hasOwnProperty.call(newComponentInputs, inputKey)) { | ||
delete (fixture.componentInstance as any)[inputKey]; | ||
} | ||
} | ||
setComponentInputs(fixture, newComponentInputs); | ||
renderedInputKeys = Object.keys(newComponentInputs); | ||
|
||
const newComponentOutputs = properties?.componentOutputs ?? {}; | ||
for (const outputKey of renderedOutputKeys) { | ||
if (!Object.prototype.hasOwnProperty.call(newComponentOutputs, outputKey)) { | ||
delete (fixture.componentInstance as any)[outputKey]; | ||
} | ||
} | ||
setComponentOutputs(fixture, newComponentOutputs); | ||
renderedOutputKeys = Object.keys(newComponentOutputs); | ||
|
||
const newComponentProps = properties?.componentProperties ?? {}; | ||
const changes = updateProps(fixture, renderedPropKeys, newComponentProps); | ||
if (hasOnChangesHook(fixture.componentInstance)) { | ||
fixture.componentInstance.ngOnChanges(changes); | ||
} | ||
renderedPropKeys = Object.keys(newComponentProps); | ||
|
||
if ( | ||
properties?.detectChangesOnRender === true || | ||
(properties?.detectChangesOnRender === undefined && detectChangesOnRender === true) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think we need this check to use the existing config? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My first intuition is to respect the initial flag from render unless explicit overwritten in rerender. But my personal goal is to never use any of those two flags - so I have no real opinion here ;) Should I only check for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea that's my personal goal as well 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done with d218907 |
||
) { | ||
fixture.componentRef.injector.get(ChangeDetectorRef).detectChanges(); | ||
} | ||
}; | ||
|
||
const changeInput = (changedInputProperties: Partial<SutType>) => { | ||
|
@@ -360,6 +392,31 @@ function getChangesObj(oldProps: Record<string, any> | null, newProps: Record<st | |
); | ||
} | ||
|
||
function updateProps<SutType>( | ||
fixture: ComponentFixture<SutType>, | ||
prevRenderedPropsKeys: string[], | ||
newProps: Record<string, any>, | ||
) { | ||
const componentInstance = fixture.componentInstance as Record<string, any>; | ||
const simpleChanges: SimpleChanges = {}; | ||
|
||
for (const key of prevRenderedPropsKeys) { | ||
if (!Object.prototype.hasOwnProperty.call(newProps, key)) { | ||
simpleChanges[key] = new SimpleChange(componentInstance[key], undefined, false); | ||
delete componentInstance[key]; | ||
} | ||
} | ||
|
||
for (const [key, value] of Object.entries(newProps)) { | ||
if (value !== componentInstance[key]) { | ||
simpleChanges[key] = new SimpleChange(componentInstance[key], value, false); | ||
} | ||
} | ||
setComponentProperties(fixture, newProps); | ||
|
||
return simpleChanges; | ||
} | ||
|
||
function addAutoDeclarations<SutType>( | ||
sut: Type<SutType> | string, | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While looking at this PR I think we should also include componentInputs to ngOnChanges.
We don't do this with the current behavior, but I think that that should be added (in a later PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. If this PR would be released without it, and I would use componentInputs in rerender, I probably would create a bug for it. If you agree I would already add it in this PR.