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 Issues #12 and #15 #26

Closed
wants to merge 3 commits into from

Conversation

rjacobskind
Copy link

Issues fixed:
#12
#15

@benwilson512
Copy link
Contributor

Hey @mgtitimoli does this address some of the issues you'd found with the other PRs?

@mpoeter
Copy link

mpoeter commented Oct 10, 2018

@rjacobskind thanks for creating this PR. I had a chat with @mgtitimoli a few weeks ago and he was not entirely happy with my solution but wanted to pursue a different approach.
Anyway, I recently found another issue in my original fix. You might wanna take a look at the last two commits at https://github.com/Lean5/absinthe-socket/commits/master and include the fix in this PR.

@rjacobskind
Copy link
Author

Thanks for the heads up @mpoeter . I'll take a look at it soon and adjust the PR accordingly

@namxam
Copy link

namxam commented Nov 8, 2018

Any update on this? We are seeing the described errors and would love to get a fix 😄

@archseer
Copy link

archseer commented Nov 9, 2018

Can we just cherry pick @mpoeter's commits directly? These commits are identical to his, but don't retain the original authorship. His branch also has a few followup fixes: https://github.com/Lean5/absinthe-socket/commits/master

We've been using that fork with no problems 👍

@rjacobskind
Copy link
Author

Feel free to cherry pick @archseer . I copied these commits and submitted a PR because several people are seeing this bug. A fix should really be incorporated into the library, rather than have everybody cherry pick the fix

@mpoeter
Copy link

mpoeter commented Nov 10, 2018

I would have created a PR myself back then, but I had a chat with @mgtitimoli via Slack and he was not happy with my solution, but wanted to pursue a different approach. I was under the impression that he wanted to provide a different fix within a few weeks, but this was almost 3 months ago and I am not aware of any progress since then.

IMHO these are serious issues in the currently released version and I absolutely agree that it is high time that a new release that fixes these issues gets published.

@rmoorman
Copy link

I can verify that the patch here (which are commits from @mpoeter I believe?) solves my issues regarding #12. It would be great to have a fix for the faulty behaviour released soon.

@mgtitimoli
Copy link
Member

Thanks for having taken the time to contribute.

Closing this as it does not address the real issue.

You can find the next release at #33.

@mgtitimoli mgtitimoli closed this Dec 10, 2018
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.

7 participants