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

Implementation of asynchronous push client #34

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

shpaker
Copy link

@shpaker shpaker commented Nov 20, 2020

No description provided.

@shpaker
Copy link
Author

shpaker commented Jan 8, 2021

?

@julianpacker
Copy link
Collaborator

julianpacker commented Feb 3, 2021

Hey, sorry about the delay reviewing. Will be reviewing within a day or 2.

Did you pull this locally and test this and confirm both the synchronous and async method work? There's no tests in the repo at the moment.

And from the looks of it you also just cleaned up the synchronous method as well, but no functional change?

In addition I think I understand what you're doing but if you can provide a slightly larger description that would be appreciated!

@SibiAkkash
Copy link
Contributor

#39 This might help in the testing process

@ahmedalshamary
Copy link

Hi, was wondering about the status of this merge request and if I could help in any way to get this merged in.

@julianpacker
Copy link
Collaborator

Hi, I was looking for reasoning of this change, and review / testing to make sure it wouldn't break current applications. Is this a change you are interested in?

If so, do you mind explaining what this change is doing?

Will also need to resolve current merge conflicts here but if everything's good, I'm able to merge / redeploy

@shpaker
Copy link
Author

shpaker commented Mar 5, 2022

Hi!
Now, i am not working with expo-sdk and i did this merge request for tasks for one startup-company (not active at current time). This patch allows to use this library with asynchronous code (AsyncClient class) and without any blocking https-requests.
It’s necessary to resolve merge conflicts, test this and merge this into the “master”.
I tried to do this yesterday but i’m afraid to break anything in code because i don’t use this now + tests are absent.

@julianpacker
Copy link
Collaborator

Okay looks like this is valuable, but would need to be tested. When I have some free time, I'll download this and test it works ok with my app before i can approve this

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.

4 participants