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

Ngrx component setup #2046

Merged
merged 63 commits into from
Mar 19, 2020
Merged

Conversation

BioPhoton
Copy link
Contributor

@BioPhoton BioPhoton commented Aug 7, 2019

Move the content to #2441

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.

This is awesome 🎉
I've found some typos in the docs 👀

projects/ngrx.io/content/guide/component/index.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/component/index.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/component/index.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/component/index.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/component/index.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/component/index.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/component/index.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/component/index.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/component/index.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/component/index.md Outdated Show resolved Hide resolved
@ngrxbot
Copy link
Collaborator

ngrxbot commented Aug 7, 2019

Preview docs changes for 82b07b8 at https://previews.ngrx.io/pr2046-82b07b8/

@BioPhoton
Copy link
Contributor Author

Thanks for the review @timdeschryver !

@jtcrowson
Copy link
Contributor

@BioPhoton Are you looking for feedback on the documentation here? I'm happy to review, if so.

@jtcrowson
Copy link
Contributor

Also, if you add

{
      "title": "Component",
      "children": [
        {
          "title": "Overview",
          "url": "guide/component"
        }
      ]
    },

to projects/ngrx.io/content/navigation.json, you'll be able to navigate to the docs from the sidebar.

@BioPhoton
Copy link
Contributor Author

@jtcrowson Yes i look for feedback on the idea. Also the pocs could be looked at. But the main thing here is the idea and features included

@brandonroberts brandonroberts added the Need Discussion Request For Comment needs input label Aug 22, 2019
Copy link

@adrael adrael left a comment

Choose a reason for hiding this comment

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

Hello there!
Fixed a few typos.

This sounds promising!
Thanks @MikeRyanDev for introducing this at AngularConnect 2019.

projects/ngrx.io/content/guide/component/index.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/component/index.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/component/index.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/component/index.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/component/index.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/component/index.md Outdated Show resolved Hide resolved
@LayZeeDK
Copy link
Contributor

Go Michael! 👏

@SerkanSipahi
Copy link
Contributor

Nice!

@BioPhoton
Copy link
Contributor Author

Hi all,

ATM I'm working on the performance optimizations with zone-less applications and the new component package.
The feature that is needed here is the coalescing of CD (change detection) calls.

I'm trying to get a good overview before I start hacking so the following information may contain miss assumptions. Please correct me wherever needed!

I already tested out some techniques with @JiaLiPassion and created a configurable RxJS operator for it. This operator acts as a rate-limiting operator on the event-loop level.

I create 2 variants, one that filters and one more like a side effect operator like a tap that fires the side effect based on some condition.

function observeOncePerAnimationFrame<T>(
  // the e.g. our CD call
  work: () => void,
  // configuration to define the scope and provide a reference to the animationFrame reference (zone patched it so we cant use `requestAnimationFrame` directly
  cfg: {
    // by default the coalescing works app-wide.
    // Key to this problem will be to find the right scoping here
    context: window,
   // in Angular zone patches the animation frame so we have to provide the unpatched one in our case
    requestAnimationFrameRef: requestAnimationFrame,
  }
)
}

// == Usage ===
changes$.pipe(
  observeOncePerAnimationFrame(this.cdRef.detectChanges)
).subscribe();

The big challenge is to find the right approach for scoping the CD coalescing.

What does it mean?

Problem:
We can not use e.g. the window as the scope in which we ignore all but one call, we have to do it per component or even more per template scope.
Multiple pipes or let directives on the same page trigger CD individually.

If we run zone-full markDirty takes over this problem and coalesces multiple calls IN a animationFrame or uses A animationFrame to do the chek. (currently, I don't have everything at the top of my head, but @LayZeeDK was so kind and reverse engineered a bit. So the correct information is here.)
However, if we run zone-less, and many motivation of | push especially, and *let, is to enable us easy to write zone-less applications.

When running zone-less Angulars can't detect the changes and so can't coalesce them, so we need to come up with our own strategy.

So if multiple changes happen at the same moment they will fire multiple CD calls.
Here we have to solve the above-mentioned problem of coalescing the CD calls within a component on our own.

Research:
Inside the pipe (or the directive), and this is independent of Ivy's ɵdetectChanges or ViewEngines ChangeDetectorRef.detectChanges we use EmbeddedViewRef.

If we start to dig in we realize there are different situations, the EmbeddedViewRef is eighter a reference to the parent view or the child component or template scope, depending on the usage.

  • Template Interpolation:
{{observable$ | push}}

This situation is valid for the push pipe. Here the pipe is used to render values to the HTML.
Inside the pipe, if we log we should see something like 'parentSomthingSomething' if we log the EmbeddedViewRef.

  • Template context:
<ng-container *let="observable$ as o"></ng-container>
<simple-component [value]="observable$ | push as o"></simple-component>

This situation is valid for the push pipe and *let directive. Here the pipe or directive is used to bind values to a specific HTML scope (the ng-content example) or to a components input binding (the simple-component example). If we log we should see something related to the child scope (please excuse me if I get vague here, but ATM I just try to put thoughts down and don't have the code open) or similar if we log the EmbeddedViewRef.

With this information, let's think about the coalescing problem again.
In the case of template interpolation, all events fired inside the scope of the component we should fire CD only once even if we fire it 3 times at the same moment.
In the case of template context, we fire CD for every scope once. Meaning multiple bindings of the same component updated at the same time should fire only once per tick.

First Thoughts:
Depending on the what our EmbeddedViewRef ref is we have to use a different scoping for the coalescing.

In the case of the push pipe placed as template interpolation, we could coalesce all CD calls within the component as only one call is needed, no matter if we bind the same observable multiple times or multiple different observables.

In the case of *let and the push pipe use as template binding, we can use it's EmbeddedViewRef pointing to the child as a reference and coalesce multiple changes within a template scope. Other *let directives next to it will trigger separately and could fire in the same tick to update it's scope of the template. This is good if we have other bindings and bad if we bind the same observable at another place in the template.

@LayZeeDK
Copy link
Contributor

LayZeeDK commented Jan 28, 2020

Unlike ChangeDetectorRef#markForCheck and ChangeDetectorRef#detectChanges, markDirty can work in zoneless (NoopNgZone) projects. I think markDirty solves all the problems you mention in Ivy. With or without an NgZone.

What does markDirty do?

Similar to ChangeDetectorRef#markForCheck, markDirty marks the component's view and all its ancestors all the way up to the root component's view as dirty. Unlike ChangeDetectorRef#markForCheck, markDirty also schedules a tick to perform a change detection cycle which dirty checks bindings based on the change detection mode (CheckOnce/CheckAlways) and dirty flag of individual component views. This tick is scheduled using a scheduler. The default scheduler for this is requestAnimationFrame, but can be configured to use a different scheduler when using renderComponent to bootstrap a new component tree.

In which cases does markDirty not solve the problems?

  1. In View Engine applications (it's recommended to keep supporting View Engine in Angular versions 9 and 10).
  2. There's a difference between markDirty and detectChanges. detectChanges synchronously runs a change detection cycle starting at the passed component's view and continues down through its descendants in this sub-component tree. I can't determine whether there might be some edge cases here. Angular's own PushPipe from CommonModule uses ChangeDetectorRef#markForCheck though (still the case in Angular version 9) and depends on ApplicationRef + NgZone to schedule an application tick (a change detection cycle starting at the root component).

How does markDirty coalesce calls into a single change detection cycle per VM turn?

markDirty--or rather a function it indirectly depends on called scheduleTick (which is unfortunately not publicly exposed)--sets a single bit the first time in a VM turn that it is called. In the same breath, it schedules a tick using the relevant scheduler (default is ´requestAnimationFrame`). In consequent calls, it checks whether the bit is set. If so, it's basically a no-op.

markDirty first executes the statement markViewDirty(getComponentViewByInstance(component)); before calling scheduleTick.

markViewDirty is the one doing the work of traversing the component tree upwards and marking views as dirty (need dirty checking, regardless of CheckOnce/CheckAlways).

The problem with detectChanges

ChangeDetectorRef#detectChanges and detectChanges are executed synchronously. This means that they can trigger change detection an infinite number of times within the same VM turn, effectively blocking the main thread.

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.

@BioPhoton I resolved some merge conflicts after bringing the ngrx branch up to date.
If you're working on it on your branch, don't forget to pull my merge commit in 🙂

Could you also clean up the unused files (did the merge go wrong?), for example we now have a push and push$.

There are also some linting errors, these can't be seen in our CI because it's currently failing. Once these comments are resolved, we should see the linting errors on the CI server.

modules/component/package-lock.json Outdated Show resolved Hide resolved
modules/component/src/component.module.ts Outdated Show resolved Hide resolved
modules/component/src/index.ts Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/component/index.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/component/index.md Outdated Show resolved Hide resolved
modules/component/src/index.ts Outdated Show resolved Hide resolved
modules/component/src/state/state.ts Outdated Show resolved Hide resolved
@BioPhoton
Copy link
Contributor Author

BioPhoton commented Feb 1, 2020

@timdeschryver There must be something completely wrong as you comment on files I deleted a long time ago. We can maybe try to get your changes cherry-picked out and merged into mine?

What should we do? I assume I have some inconsistencies on my side too and therefore thewhole thing happened.

#2046 (comment)

The hooks thing is e.g. one thing I deleted I guess a month ago or so...

@BioPhoton
Copy link
Contributor Author

@LayZeeDK

The described situation works for zone-full applications for sure.

For zone-less I'm not sure. I will invest in this in the next days.

Also markDirty schedules an app.tick. This would trigger rerendering (or at least the check for it) everywhere? right?

@LayZeeDK
Copy link
Contributor

LayZeeDK commented Feb 1, 2020

Also markDirty schedules an app.tick. This would trigger rerendering (or at least the check for it) everywhere? right?

Yes, the internal scheduleTick function used by ɵmarkDirty will schedule a change detection cycle for the full component tree, starting at the root component. ChangeDetectorRef#detectChanges and ɵdetectChanges will run a change detection cycle at the sub component tree that has the specified component at its root.

@LayZeeDK
Copy link
Contributor

LayZeeDK commented Feb 1, 2020

@JoostK @alexzuza What do you think about ɵdetectChanges vs. ɵmarkDirty with/without NgZone? Performance? Synchronicity of ɵdetectChanges? Coalescing to a single change detection cycle per VM turn?

@BioPhoton
Copy link
Contributor Author

BioPhoton commented Feb 1, 2020

Important to say here is that we can coalesce the detectChanges call within a component or embedded view: #2046 (comment)
Not only VM turn

@LayZeeDK
Copy link
Contributor

LayZeeDK commented Feb 1, 2020

within a compoent or embedded view

Why is that, Michael?

@LayZeeDK
Copy link
Contributor

LayZeeDK commented Feb 1, 2020

If we run zone-full markDirty takes over this problem and coalesces multiple calls IN a animationFrame or uses A animationFrame to do the chek. (currently, I don't have everything at the top of my head, but @LayZeeDK was so kind and reverse engineered a bit. So the correct information is here.)
However, if we run zone-less, and many motivation of | push especially, and *let, is to enable us easy to write zone-less applications.

When running zone-less Angulars can't detect the changes and so can't coalesce them, so we need to come up with our own strategy.

Not true. ɵmarkDirty would work equally well with NoopNgZone. ChangeDetectorRef#markForCheck would not though.

Two main issues to be aware of:

  1. ChangeDetectorRef#markForCheck doesn't schedule a tick.
  2. ChangeDetectorRef#detectChanges doesn't coalesce and dirty checks synchronously since it executes immediately instead of scheduling.

@BioPhoton
Copy link
Contributor Author

BioPhoton commented Feb 2, 2020

within a component or embedded view

Why is that, Michael?

Please correct me if needed.

{{observable$ | push}}

revieves a different cdRef (EmbeddedView) than

<ng-container *ngIf="observable$ | push as o">
{{o}}
</ng-container>

does.

The first is meant by component the second by embedded view.

@LayZeeDK
Copy link
Contributor

LayZeeDK commented Feb 2, 2020

Please correct me if needed.

{{observable$ | push}}

revieves a different cdRef (EmbeddedView) than

<ng-container *ngIf="observable$ | push as o">
{{o}}
</ng-container>

does.

The first is meant by component the second by embedded view.

I just ran another experiment to rediscover what I learned when we were trying to manage change detection without Zone.js.

In Ivy

Part ChangeDetectorRef._cdRefInjectingView
Component LComponentView_*
Pipe in template expression LComponentView_* (the same object as for the component)
Pipe in NgIf expression LEmbeddedView_*

They are all separate instances of a ChangeDetectorRef implementation (this is also true in View Engine)

@BioPhoton
Copy link
Contributor Author

@LayZeeDK

Ha!! I had the right instinct!

They each have a separate instance of a ChangeDetectorRef implementation (this is also true in View Engine)

What about directives (*ngrxLet)?

Also, I don't see the difference between component and pipe in template expression.

It's hard for me to find but I assume that ChangeDectorRef will use ɵdetectChanges under the hood. So I will focus to solve everything with cdRef.

@LayZeeDK
Copy link
Contributor

LayZeeDK commented Feb 2, 2020

*ngrxLet will be similar to *ngIf. No difference between regular template expression or pipe in template expression, it seems. Except that we can't inject a ViewRef in a pipe, isn't that what we discovered?

@BioPhoton
Copy link
Contributor Author

Update:
@brandonroberts Done with resolving the review.
Please let me know how to proceed.

Demo here:
https://github.com/BioPhoton/angular-zone-less-examples

@BioPhoton
Copy link
Contributor Author

BioPhoton commented Mar 17, 2020

Hi @alexzuza,

The issue: angular/angular#33677
is still a topic. I followed all other links but the smart angular bot closed all issues so I can't follow up on the discussion. :D

There are 2 PRs I found provided a solution for it. None of them merged.

What is your status? Do you know more?

Thanks, Michael!

@brandonroberts
Copy link
Member

Thanks for the work @BioPhoton! As discussed, we are proceeding with merging this in, and following up with additional PRs for cleanup.

@BioPhoton BioPhoton requested a review from alex-okrushko March 19, 2020 18:57
@brandonroberts brandonroberts merged commit 464073d into ngrx:master Mar 19, 2020
* Included Features:
* - Take observables or promises, retrieve their values and render the value to the template
* - Handling null and undefined values in a clean unified/structured way
* - Triggers change-detection differently if `zone.js` is present or not (`detectChanges` or `markForCheck`)

Choose a reason for hiding this comment

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

@BioPhoton PushPipe in zone-less mode uses detectChanges which triggers change detection synchronously. Isn’t it better to markForCheck and schedule CD (appref.tick) on at least the next requestAnimationFrame?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @eugene-stativka !

Definitely!

I summed everything related to this problem in a document here:
https://hackmd.io/42oAMrzYReizA65AwQ7ZlQ

Please let me know the feedback here: #2441

@sameh-g
Copy link

sameh-g commented Oct 19, 2022

Is it OK to start use this lib in my Project ?? Since its still under development or ?? @BioPhoton

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Need Discussion Request For Comment needs input
Projects
None yet
Development

Successfully merging this pull request may close these issues.