Skip to content

Conversation

@peppedeka
Copy link
Contributor

No description provided.

@peppedeka peppedeka force-pushed the refactoring-app-component branch from 60d7447 to 6b9882a Compare December 8, 2019 11:31
@peppedeka peppedeka force-pushed the refactoring-app-component branch from 6b9882a to 044b58f Compare December 8, 2019 11:33
@andrea82
Copy link
Member

andrea82 commented Dec 12, 2019

Hi @peppedeka
what about using merge operator for commands response and clear actions?
Something like this:
this.responses$ = merge(this.redisConnectService.response$, this.currentResponseBs.asObservable()).pipe( scan((a, c) => c != null ? [...a, c] : [], []), );

in this way we can remove the subscription inside the component .ts file, method destroy and lines from 23 to 25 as well.
Do you think is a good idea?

@peppedeka
Copy link
Contributor Author

peppedeka commented Dec 14, 2019

Good idea,
i removed useless subscription and implementation of ngDestroy.
i'm follow you hint, used static merge operators but i applied catchError operator for handling external service call (because inject var can be undefined and generate error inside merge that can be break the flow)
I don't remove currentResponse$ getter because is used by async pipe inside the app.component template

@andrea82
Copy link
Member

Yes absolutely right,
we still have to add an error handler so for now your fix is fine, but wouldn't it be better to handle the error in RedisConnectService service?
Sorry but i've not found currentResponse$ in app.component template, if it is necessary keep it, otherwise remove.

Thanks so much!

@peppedeka
Copy link
Contributor Author

Sounds good!
i moved observables catching error to redis-connect.service
yes :) currentResponse$ is useless i removed it

@andrea82 andrea82 merged commit 34c70e6 into acadevmy:develop Dec 16, 2019
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