fix(fxLayoutGap): mutation observer should run outside the ngZone #370
Conversation
|
@crisbeto - would you mind reviewing these? |
src/lib/flexbox/api/layout-gap.ts
Outdated
| @@ -15,7 +15,7 @@ import { | |||
| Self, | |||
| AfterContentInit, | |||
| Optional, | |||
| OnDestroy, | |||
| OnDestroy, NgZone, | |||
There was a problem hiding this comment.
This one should be on the next line.
src/lib/flexbox/api/layout-gap.ts
Outdated
| this._zone.runOutsideAngular(() => { | ||
|
|
||
| let onMutationCallback = (mutations) => { | ||
| let validatedChanges = (it: MutationRecord) => { |
There was a problem hiding this comment.
Since this is only used once it can be inlined, but I guess that's down to preference. Same goes for the onMutationCallback.
src/lib/flexbox/api/layout-gap.ts
Outdated
| }; | ||
|
|
||
| // update gap styles only for child 'added' or 'removed' events | ||
| if (mutations.filter(validatedChanges).length) { |
There was a problem hiding this comment.
Instead of using mutation.filter(...).length, you can use mutations.some(...) which will exit earlier.
src/lib/flexbox/api/layout-gap.ts
Outdated
| if (mutations.filter(validatedChanges).length) { | ||
| this._updateWithValue(); | ||
| if (typeof MutationObserver !== 'undefined') { | ||
| this._observer = new MutationObserver(onMutationCallback.bind(this)); |
There was a problem hiding this comment.
The bind(this) shouldn't be necessary since it's an arrow function.
src/lib/flexbox/api/layout-gap.ts
Outdated
| this._zone.runOutsideAngular(() => { | ||
|
|
||
| let onMutationCallback = (mutations) => { | ||
| let validatedChanges = (it: MutationRecord) => { |
There was a problem hiding this comment.
I believe this is a MutationRecord[], not a single MutationRecord.
Inside the ngZone, the mutation observer and _updateWithValue() style changes causes an infinite recursion. MutationObserver handlers should be executed outside the ngZone. Fixes #329.
ThomasBurleson
force-pushed
the
thomas/issue-329
branch
from
fe9d020 to
23f4f1d
Compare
|
@crisbeto - nice feedback; as usually. Changes complete. |
| this._updateWithValue(); | ||
| this._zone.runOutsideAngular(() => { | ||
|
|
||
| if (typeof MutationObserver !== 'undefined') { |
There was a problem hiding this comment.
Not a big deal, but you can move the MutationObserver support check outside of the runOutsideAngular to avoid hitting runOutsideAngular completely if it's unsupported.
There was a problem hiding this comment.
true. minor nit. agreed ?
There was a problem hiding this comment.
Yeah it's pretty minor, no need to change it.
ThomasBurleson
added
pr: lgtm
* Runs the underlying `MutationObserver` outside the Angular zone. * Runs the debounce timeout outside the Angular zone. via angular/flex-layout#370.
* Runs the underlying `MutationObserver` outside the Angular zone. * Runs the debounce timeout outside the Angular zone. via angular/flex-layout#370.
* Runs the underlying `MutationObserver` outside the Angular zone. * Runs the debounce timeout outside the Angular zone. via angular/flex-layout#370.
* Runs the underlying `MutationObserver` outside the Angular zone. * Runs the debounce timeout outside the Angular zone. via angular/flex-layout#370.
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Inside the ngZone, the mutation observer and _updateWithValue() style changes causes an infinite recursion. MutationObserver handlers should be executed outside the ngZone.
Fixes #329.