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

AAE-26189 Replaced observable based counter to avoid flickering #10290

Merged
merged 2 commits into from
Oct 11, 2024

Conversation

tomaszhanaj
Copy link
Contributor

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation
  • Other... Please describe:

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

  • Yes
  • No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

[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] }}
Copy link
Contributor

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

Copy link
Contributor Author

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">

image

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:

  1. create elements
    image
    2.Initiate them when filters are fetched
    image
    3.get data from server and make it an observable (stream)
    image
    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

Copy link
Contributor

@VitoAlbano VitoAlbano Oct 10, 2024

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

@tomaszhanaj tomaszhanaj force-pushed the fix/AAE-26189-flickering-numbers branch from dcfd622 to 2f6ed59 Compare October 10, 2024 09:55
Copy link

@tomaszhanaj tomaszhanaj merged commit eb12083 into develop Oct 11, 2024
17 checks passed
@tomaszhanaj tomaszhanaj deleted the fix/AAE-26189-flickering-numbers branch October 11, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants