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

Refactor of Websocket wrapper to use promise. #207

Merged
merged 3 commits into from
Nov 21, 2022

Conversation

dlabordus
Copy link

@juancho0202 as already told a nicer version for using websocket.

The websocket function now returns a Promise like most rest calls are also doing after some response handling.
If something goes wrong a reject is done with the reason body as used in other places with rest calls.
This way the handling if websocket or rest should be used could be hidden in the service layer.
Calling code doesn't know anymore.

Remark: I created a local container image and used the Docker Compose file and robot test script to validate the change.

Signed-off-by: Dennis Labordus <dennis.labordus@alliander.com>
@dlabordus dlabordus self-assigned this Nov 17, 2022
@dlabordus dlabordus added the enhancement New feature or request label Nov 17, 2022
Signed-off-by: Dennis Labordus <dennis.labordus@alliander.com>
juancho0202
juancho0202 previously approved these changes Nov 17, 2022
Copy link

@juancho0202 juancho0202 left a comment

Choose a reason for hiding this comment

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

I really like how you got rid of the _UsingWebsockets and _UsingRest in so many components. Code is defo more straight-forward. My only comment I consider non-blocking, therefore I approved already.

Signed-off-by: Dennis Labordus <dennis.labordus@alliander.com>
@dlabordus dlabordus merged commit 442ddc2 into main Nov 21, 2022
@dlabordus dlabordus deleted the refactor-websocket-wrapper branch November 21, 2022 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants