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.

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

fix(429): markForCheck when calling event handler #430

Merged
merged 1 commit into from
Jan 16, 2021
Merged

Conversation

ajafff
Copy link
Contributor

@ajafff ajafff commented Jan 15, 2021

IoService calls markForCheck on the on the host component's change detector before an event handler is called.
It's done before calling the event handler on purpose: this way the flag is reset the handler calls detectChanges and there's no additional change detection after the current tick. This also seems to be what Angular does in templates.

Note that I had difficulties understanding the tests. Therefore I duplicated a bit of code to make my tests work without affecting everything else.

Fixes: #429

@vercel
Copy link

vercel bot commented Jan 15, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/gund/ng-dynamic-component/kf9ppyvme
✅ Preview: Failed

[Deployment for ef55884 failed]

@vercel vercel bot temporarily deployed to Preview January 15, 2021 18:32 Inactive
@codecov
Copy link

codecov bot commented Jan 15, 2021

Codecov Report

Merging #430 (ef55884) into master (d4e87ee) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #430   +/-   ##
=======================================
  Coverage   99.15%   99.15%           
=======================================
  Files          16       16           
  Lines         353      356    +3     
  Branches       60       60           
=======================================
+ Hits          350      353    +3     
  Misses          2        2           
  Partials        1        1           
Impacted Files Coverage Δ
...dynamic-directives/dynamic-directives.directive.ts 97.72% <ø> (ø)
...dynamic-component/src/lib/io/io-factory.service.ts 100.00% <100.00%> (ø)
...ects/ng-dynamic-component/src/lib/io/io.service.ts 99.27% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d42111e...ef55884. Read the comment docs.

Copy link
Owner

@gund gund left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Great job with tests as well.
I have just one comment to address below.

processedOutputs[key] = outputExpr;
processedOutputs[key] = event => {
// Mark host component for check every time the event handler is called
this.cdr?.markForCheck();
Copy link
Owner

@gund gund Jan 15, 2021

Choose a reason for hiding this comment

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

Instead of having CDR checks in 2 places you can move it into single place in the bindOutputs() method where the subscriptions are created:

.subscribe((event: any) => (outputs[p] as EventHandler)(event)),

Also you should first try to use dynamic component's CDR (available in compCdr prop) and if not available then fallback to it's own injected CDR.

private get compCdr(): ChangeDetectorRef {
// tslint:disable-next-line: deprecation
return this.compRef ? this.compRef.injector.get(ChangeDetectorRef) : null;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved it to bindOutputs.

I tried using compCdr, but that didn't work. Always using the host's ChangeDetectorRef is also more correct, because the injected component does not need another change detection. Only the component that handles the event needs to be change detected again.

Copy link
Owner

Choose a reason for hiding this comment

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

Sounds logical, I just thought that marking the dynamic component itself should yield the same result as marking parent - because it's located under parent in the tree so it should become marked as every other parent component...
Don't quite understand why it does not mark parent component.
Maybe you know the reason?

@vercel vercel bot temporarily deployed to Preview January 16, 2021 07:49 Inactive
Copy link
Owner

@gund gund left a comment

Choose a reason for hiding this comment

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

Thanks for changes - looks great!

@gund gund merged commit 2a262d2 into gund:master Jan 16, 2021
gund pushed a commit that referenced this pull request Jan 16, 2021
## [8.0.1](v8.0.0...v8.0.1) (2021-01-16)

### Bug Fixes

* **io:** invoke markForCheck when output handler is called ([2a262d2](2a262d2)), closes [#430](#430)
@gund
Copy link
Owner

gund commented Jan 16, 2021

🎉 This PR is included in version 8.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gund gund added the released label Jan 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ndcDynamicOutputs: call markForCheck after handler is called
2 participants