feat: support ngOnChanges with correct simpleChange object within rerender #366

shaman-apprentice · 2023-02-11T21:34:43Z

feat: support ngOnChanges with correct simpleChange object within rerender

close #365

any feedback or changes are highly appreciated :)

shaman-apprentice · 2023-02-11T22:09:58Z

Help regarding the pipeline is appreciated. I had the same error as the pipeline locally. After I touched and restored the tsconfig.spec.json it works for me locally. I added locally an assertion to be sure it doesn't use nx's cache.

~~Is there maybe something miss-configured with nx? Unfortunately I have no experiences with it.~~

see #370

timdeschryver

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

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

shaman-apprentice · 2023-02-21T20:34:55Z

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? 🤔

timdeschryver · 2023-02-22T15:55:46Z

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

timdeschryver · 2023-02-22T17:40:26Z

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

shaman-apprentice · 2023-02-22T22:36:50Z

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

timdeschryver · 2023-02-23T09:56:07Z

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

…render ref testing-library#365

Co-authored-by: Tim Deschryver <[email protected]>

timdeschryver

Looks good, just one question (sorry 😅)

timdeschryver · 2023-02-28T18:02:13Z

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


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

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.

timdeschryver · 2023-02-28T18:03:17Z

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

+    }
+    renderedPropKeys = Object.keys(newComponentProps);
+
+    fixture.componentRef.injector.get(ChangeDetectorRef).detectChanges();


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?

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

shaman-apprentice · 2023-03-02T18:57:45Z

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.

timdeschryver · 2023-03-02T19:53:29Z

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

ref testing-library#365

timdeschryver · 2023-03-07T19:06:57Z

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

-    fixture.componentRef.injector.get(ChangeDetectorRef).detectChanges();
+    if (
+      properties?.detectChangesOnRender === true ||
+      (properties?.detectChangesOnRender === undefined && detectChangesOnRender === true)


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.

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?

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

done with d218907

ref testing-library#365

timdeschryver

Thanks for your work on this @shaman-apprentice ! ❤️

shaman-apprentice mentioned this pull request Feb 19, 2023

fix: add types for @testing-library/jasmine-dom #370

Merged

shaman-apprentice force-pushed the feat/365/support-ngonchanges-in-rerender branch from 72e5e9a to f0077e8 Compare February 20, 2023 16:52

timdeschryver reviewed Feb 21, 2023

View reviewed changes

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

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

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

shaman-apprentice and others added 2 commits February 23, 2023 18:21

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 <[email protected]>

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

View reviewed changes

feat: support detectChangesOnRender within rerender

4c5a255

ref testing-library#365

timdeschryver reviewed Mar 7, 2023

View reviewed changes

feat: only check for own detectChangesOnRender flag

d218907

ref testing-library#365

timdeschryver approved these changes Mar 10, 2023

View reviewed changes

timdeschryver merged commit 83c323f into testing-library:main Mar 10, 2023

dzonatan mentioned this pull request Jul 18, 2023

docs(angular): update rerender api testing-library/testing-library-docs#1280

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support ngOnChanges with correct simpleChange object within rerender #366

feat: support ngOnChanges with correct simpleChange object within rerender #366

shaman-apprentice commented Feb 11, 2023

shaman-apprentice commented Feb 11, 2023 •

edited

Loading

timdeschryver left a comment

shaman-apprentice commented Feb 21, 2023 •

edited

Loading

timdeschryver commented Feb 22, 2023

timdeschryver commented Feb 22, 2023

shaman-apprentice commented Feb 22, 2023

timdeschryver commented Feb 23, 2023

timdeschryver left a comment

timdeschryver Feb 28, 2023

shaman-apprentice Mar 5, 2023

timdeschryver Feb 28, 2023

shaman-apprentice Mar 5, 2023

shaman-apprentice commented Mar 2, 2023

timdeschryver commented Mar 2, 2023

timdeschryver Mar 7, 2023

shaman-apprentice Mar 7, 2023 •

edited

Loading

timdeschryver Mar 8, 2023

shaman-apprentice Mar 10, 2023 •

edited

Loading

timdeschryver left a comment

feat: support ngOnChanges with correct simpleChange object within rerender #366

feat: support ngOnChanges with correct simpleChange object within rerender #366

Conversation

shaman-apprentice commented Feb 11, 2023

shaman-apprentice commented Feb 11, 2023 • edited Loading

timdeschryver left a comment

Choose a reason for hiding this comment

shaman-apprentice commented Feb 21, 2023 • edited Loading

timdeschryver commented Feb 22, 2023

timdeschryver commented Feb 22, 2023

shaman-apprentice commented Feb 22, 2023

timdeschryver commented Feb 23, 2023

timdeschryver left a comment

Choose a reason for hiding this comment

timdeschryver Feb 28, 2023

Choose a reason for hiding this comment

shaman-apprentice Mar 5, 2023

Choose a reason for hiding this comment

timdeschryver Feb 28, 2023

Choose a reason for hiding this comment

shaman-apprentice Mar 5, 2023

Choose a reason for hiding this comment

shaman-apprentice commented Mar 2, 2023

timdeschryver commented Mar 2, 2023

timdeschryver Mar 7, 2023

Choose a reason for hiding this comment

shaman-apprentice Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

timdeschryver Mar 8, 2023

Choose a reason for hiding this comment

shaman-apprentice Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

timdeschryver left a comment

Choose a reason for hiding this comment

shaman-apprentice commented Feb 11, 2023 •

edited

Loading

shaman-apprentice commented Feb 21, 2023 •

edited

Loading

shaman-apprentice Mar 7, 2023 •

edited

Loading

shaman-apprentice Mar 10, 2023 •

edited

Loading