Search code, repositories, users, issues, pull requests...

timdeschryver reviewed

Feb 21, 2023

Copy link

Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

Thanks, I like the change!
I just left a few remarks.
I was also thinking if this could be a breaking change 🤔 what do you think?

projects/testing-library/tests/rerender.spec.ts Outdated Show resolved Hide resolved

projects/testing-library/src/lib/models.ts Outdated Show resolved Hide resolved

Copy link

Author

shaman-apprentice commented Feb 21, 2023 •

edited

Loading

I was also thinking if this could be a breaking change what do you think?

I thought about that as well and I am not sure:

From my React-based experiences this now works as expected. We also don't need to change any existing test assertions. So regarding semantic versioning my gut feeling is a fix.

I went with feature, as now testing of ngOnChanges is supported within rerender.

But if e.g. someone spies and asserts numbers of calls of a functions, which gets called in constructor, his tests would now fail, as constructor isn't called again in rerender anymore.

So it's probably most consumer friendly to defensively declare a breaking change? 🤔

Copy link

Member

timdeschryver commented Feb 22, 2023

@shaman-apprentice I kind of want this to be a feature as well.
I'm thinking of doing a patch version first with the deprecations.
And then a minor with the rerender changes.

The next breaking would remove the other two methods + upgrade to DOM v9.

Copy link

Member

timdeschryver commented Feb 22, 2023

Do you want to deprecate the methods in a new PR @shaman-apprentice ?
Otherwise I will make those changes and rebase into your branch

Copy link

Author

shaman-apprentice commented Feb 22, 2023

Do you want to deprecate the methods in a new PR @shaman-apprentice ? Otherwise I will make those changes and rebase into your branch

Here it is: #371

In case it is less work for you, feel free to directly copy paste any change you like. Otherwise I will glady continue to update. Thanks for all your work and fast replies :)

Copy link

Member

timdeschryver commented Feb 23, 2023

@shaman-apprentice if you could rebase this branch against main then we're good to go :)
Thanks a lot!

shaman-apprentice and others added 2 commits

February 23, 2023 18:21

@shaman-apprentice @timdeschryver


 feat: support ngOnChanges with correct simple change object within re...

e4acbbd

...render
ref testing-library#365


 Update projects/testing-library/tests/rerender.spec.ts

66ba0c7

Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>

@shaman-apprentice shaman-apprentice force-pushed the feat/365/support-ngonchanges-in-rerender branch from 5f42708 to 66ba0c7 Compare

February 23, 2023 17:22

timdeschryver approved these changes

Feb 28, 2023

Copy link

Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

Looks good, just one question (sorry 😅)

projects/testing-library/src/lib/testing-library.ts

const newComponentProps = properties?.componentProperties ?? {};

const changes = updateProps(fixture, renderedPropKeys, newComponentProps);

if (hasOnChangesHook(fixture.componentInstance)) {

fixture.componentInstance.ngOnChanges(changes);

Copy link

Member

@timdeschryver timdeschryver Feb 28, 2023

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).

Copy link

Author

@shaman-apprentice shaman-apprentice Mar 5, 2023

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.

projects/testing-library/src/lib/testing-library.ts Outdated

}

renderedPropKeys = Object.keys(newComponentProps);

fixture.componentRef.injector.get(ChangeDetectorRef).detectChanges();

Copy link

Member

@timdeschryver timdeschryver Feb 28, 2023

Choose a reason for hiding this comment

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

Could you also provide a parameter for this?
With render we can disable detectChanges, I suppose some tests would also require this with rerender.
Thoughts?

Copy link

Author

@shaman-apprentice shaman-apprentice Mar 5, 2023

Choose a reason for hiding this comment

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

Have I understood the concept correctly with 4c5a255?

I have mixed feelings about it. I want help to provide a nice helpful library. On the other side I am not sure it is a good idea to disable change detection of a component. I just assume the component under test has a design flaw when this is helpful. Or I am just lacking imagination of good use cases.^^

Copy link

Author

shaman-apprentice commented Mar 2, 2023

Looks good, just one question (sorry 😅)

I will gladly take a look into it. Not sure I will mange this weekend but I will come to it.

Copy link

Member

timdeschryver commented Mar 2, 2023

Np, take your time @shaman-apprentice I'm grateful for these contributions 🤗


 feat: support detectChangesOnRender within rerender

4c5a255

ref testing-library#365

timdeschryver reviewed

Mar 7, 2023

projects/testing-library/src/lib/testing-library.ts Outdated

fixture.componentRef.injector.get(ChangeDetectorRef).detectChanges();

if (

properties?.detectChangesOnRender === true ||

(properties?.detectChangesOnRender === undefined && detectChangesOnRender === true)

Copy link

Member

@timdeschryver timdeschryver Mar 7, 2023

Choose a reason for hiding this comment

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

Do you think we need this check to use the existing config?
I didn't think of it, and I assumed it would only use the property passed to this function.

Copy link

Author

@shaman-apprentice shaman-apprentice Mar 7, 2023 •

edited

Loading

Choose a reason for hiding this comment

The 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 properties?.detectChangesOnRender === true?

Copy link

Member

@timdeschryver timdeschryver Mar 8, 2023

Choose a reason for hiding this comment

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

Yea that's my personal goal as well 😅
I would only check for the properties if you don't mind.

Copy link

Author

@shaman-apprentice shaman-apprentice Mar 10, 2023 •

edited

Loading

Choose a reason for hiding this comment

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

done with d218907


 feat: only check for own detectChangesOnRender flag

d218907

ref testing-library#365

timdeschryver approved these changes

Mar 10, 2023