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

Fix: Avoid error from null value for observer in timeoutStartSubscriptionAck function #595

Merged
merged 1 commit into from
Jan 8, 2021

Conversation

KvNGCzA
Copy link
Contributor

@KvNGCzA KvNGCzA commented Oct 5, 2020

Issue #, if available:
#515 #544 #596

Description of changes:
An if statement to make sure that the observer exists before it tries to call the properties of observer in timeoutStartSubscriptionAck function

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@KvNGCzA KvNGCzA changed the title avoid null value for observer in timeoutStartSubscriptionAck function Fix: avoid error from null value for observer in timeoutStartSubscriptionAck function Oct 5, 2020
@KvNGCzA KvNGCzA changed the title Fix: avoid error from null value for observer in timeoutStartSubscriptionAck function Fix: Avoid error from null value for observer in timeoutStartSubscriptionAck function Oct 5, 2020
@99darwin
Copy link

99darwin commented Nov 4, 2020

Please pull this in!

@ankur014
Copy link

ankur014 commented Nov 5, 2020

There have been multiple issues opened related to this, can we get this merged, please?

@loganbertrand
Copy link

Can we please merge this?

@randomslap
Copy link

Can this pull request get merged please? Subscriptions are unstable and it causes app to crash due to this issue.

@jcbdev
Copy link

jcbdev commented Nov 9, 2020

This issue also exists in the timeoutDIsconnect function too!

AppSyncRealTimeSubscriptionHandshakeLink.prototype._timeoutDisconnect = function () {
        this.subscriptionObserverMap.forEach(function (_a) {
            var observer = _a.observer;
            observer.error({
                errors: [__assign({}, new graphql_1.GraphQLError("Timeout disconnect"))]
            });
            observer.complete();
        });
        this.subscriptionObserverMap = new Map();
        if (this.awsRealTimeSocket) {
            this.awsRealTimeSocket.close();
        }
        this.socketStatus = types_1.SOCKET_STATUS.CLOSED;
    };

Would be good if we could cover both issues?

@ankur014
Copy link

There hasn't been any traction on this for some time, so, we tried this workaround and it seems to be working. Adding it here so that others can give it a shot as well - https://dev.to/willsamu/how-to-get-aws-appsync-running-with-offline-support-and-react-hooks-678

@KvNGCzA
Copy link
Contributor Author

KvNGCzA commented Dec 21, 2020

Hey @jcbdev . I opened this to fix the issue in the timeoutDisconnect function. Thanks for the suggestion.

@sammartinez @elorzafe Hey guys. This PR has been open for a while, can we please get it merged?

@99darwin
Copy link

@elorzafe are you still monitoring this? the best Holiday gift you could give us all would be merging this PR! 🎄🎅

@aehmt
Copy link

aehmt commented Jan 7, 2021

@elorzafe are you still monitoring this? the best Holiday gift you could give us all would be merging this PR! 🎄🎅

What's the hold up with this? Can someone from AWS explain?

@KvNGCzA
Copy link
Contributor Author

KvNGCzA commented Jan 8, 2021

@sammartinez Hi, have I missed a step in opening this PR? I read and followed the contributing guidelines. Maybe I missed something. Kindly give feedback on the next steps.

Copy link
Contributor

@elorzafe elorzafe left a comment

Choose a reason for hiding this comment

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

@KvNGCzA thanks for this PR.

I am sorry for the delay. We will do a release of this ASAP

@elorzafe elorzafe merged commit d986cb3 into awslabs:master Jan 8, 2021
@KvNGCzA
Copy link
Contributor Author

KvNGCzA commented Jan 8, 2021

@elorzafe Thank you for your response. Cheers.

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.

None yet

8 participants