-
Notifications
You must be signed in to change notification settings - Fork 269
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
AAE-26189 Replaced observable based counter to avoid flickering #10290
Conversation
[attr.data-automation-id]="filter.key + '_filter-counter'" | ||
class="adf-process-filters__entry-counter" | ||
[class.adf-active]="isFilterUpdated(filter.key)" | ||
> | ||
{{ counters$[filter.key] | async }} | ||
{{ counters[filter.key] }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like this approach as it makes the whole thing less reactive, as we discussed we can tackle this issue on a different perspective
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VitoAlbano
In reactive architecture we have:
- smart/container component - handle logic, manage data
-dumb/presentational component - present data in ui, handle user interaction and pass information about this fact to component that deals with it, so when user clicks on something, click event it is emitted (and smart component will handle it by calling an api, dispatching an action, changing routing, etc)
-state - place where we keep data for application (let it be ngrx)
In ideal example:
1.user clicks on button, event it is emitted
2.Smart component handles click and "decides" what to do with this fact, for example is dispatching action which triggers effect.
3.Effect is responsible for fetching data from server (this is not a stream, we take data and we close it). When data arrives (or not) details about it are stored in state.
4.Smart component which is interested in this particular data has already subscribed to selector (this is a stream) and when store is updated smart component receives data (it does not care who, when and why changed it).
5.Smart component is passing this data to dumb components. In more detailed way this point may be done like: in smart data$ = selector and if dumb component is using ChangeDetectionStrategy.OnPush it is very beneficiary in terms of performance.
<ng-container *ngIf="data$ | async as details"
<dumb-component [data]="details">
In our example the initiator of action is a notification from websocket. After we received notification we are fetching data for counters (get exact number of running process, number of closed processes, etc.). When data arrives we need to store it and pass it to template. To make this data an observable/stream (to use async in template and avoiding flickering when using observables) I need to:
- create elements
2.Initiate them when filters are fetched
3.get data from server and make it an observable (stream)
4.then I can use async pipe in template and avoid flickering with observables
TL;DR
In conclusion making it more reactive in this way is bad idea because:
- instead of having one object that holds primitive value like number I will have two objects holding complex objects (this is not efficient and against KISS)
- moving it to service is more cumbersome, less intuitive, nobody will understand why it happened, it will be more complicated, and will cause that logic is shared between component and service (breaking separation of concerns and single responsibility principle and KISS)
-providing reactive architecture in our example requires more deep refactoring and providing elements that does not exist in current solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pattern you described is more an ngrx pattern which is not the case on the ADF side (we don't have the state here but on the other side). The idea here was to ;
1 . Moving the set into the service to have it in a single place as it's the same and it's not part of the single filter knowing the status of the other
2. using an observable with a select filter and distinct to fetch and subscribe from the component just the changes that you are interested in
3. You can also create an extra service which can handle this status it would have been fine
4. Initialization of the filters can be done into the constructor or the init of the filter so that a new filter when coming in register its presence
To sum up what i was suggesting was a way to have less code and checks around as it is in your current solution but rely more on the operators and the rxjs .
It might also be that there are cases in which this solution won't fly (it happens i didn't do a detailed double check on the whole part to be sure) but i'm sure it can be managed.
If you have other doubts let's discuss about it , i see that i've explained what i request poorly so we can always have a refresh catch up meeting
dcfd622
to
2f6ed59
Compare
Quality Gate passedIssues Measures |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x")
What is the current behaviour? (You can also link to an open issue here)
https://hyland.atlassian.net/browse/AAE-26189
What is the new behaviour?
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information: