search

absinthe-graphql / absinthe-socket

Core JavaScript support for Absinthe WS-based operations
MIT License
150 stars 75 forks source link

Fix Issues #12 and #15 #26

Closed rjacobskind closed 5 years ago

rjacobskind commented 6 years ago

Issues fixed: https://github.com/absinthe-graphql/absinthe-socket/issues/12 https://github.com/absinthe-graphql/absinthe-socket/issues/15

benwilson512 commented 6 years ago

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

mpoeter commented 6 years ago

@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 commented 6 years ago

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

namxam commented 6 years ago

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

archseer commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 5 years ago

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.