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

Make it possible to use multiple websockets and callbacks #40

Merged
merged 2 commits into from
Nov 29, 2017

Conversation

edennis
Copy link
Contributor

@edennis edennis commented Nov 27, 2017

As described in #33 the internal state of the effects manager wasn't equipped to handle callbacks for multiple websockets. Using a tuple of the Endpoint and Ref now makes that possible.

I also had to fix the broken documentation in Phoenix.Presence to make elm-make stop complaining. The extra whitespace in the diff is due to elm-format.

@markhamburg
Copy link

This will be more efficient in general if the callback dictionary is keyed as ( ref, endpoint ) instead of ( endpoint, ref ) because it is more likely that the refs will be different than it is that the endpoints will be different. But otherwise, this looks correct.

@edennis
Copy link
Contributor Author

edennis commented Nov 28, 2017

I changed the ordering in the tuple to optimize comparisons in the Dict. For the other readers, the relevant code that handles comparisons in Elm is: https://github.com/elm-lang/core/blob/5.1.1/src/Native/Utils.js#L165-L179

@markhamburg, were you able to try the code out on your example?

* This makes it possible to use the library with more
  than one endpoint that has the same Channel.on* callback
* Extra whitespace is due to elm-format
@saschatimme
Copy link
Owner

This looks really good @edennis! I think this is ready to get merged if @markhamburg can verify that this resolves his problem.

@edennis
Copy link
Contributor Author

edennis commented Nov 28, 2017

@saschatimme I'll see if I can put together a simple example app that will show the "before" and "after" behavior.

@edennis
Copy link
Contributor Author

edennis commented Nov 28, 2017

@saschatimme I've pushed a small example app that demonstrates the broken behavior in the current version. If you run it as is you'll see the error I've described in the README. It's a bit of a pain to test my PR since you'll have to clone it locally. What I did was more or less:

cd assets/elm-stuff/packages/saschatimme/elm-phoenix
mv 0.3.1 0.3.1-old
ln -s location-of-my-pr 0.3.1

and then recompile and run again. Hope this helps convince you.

@saschatimme
Copy link
Owner

awesome work @edennis! :)

@saschatimme saschatimme merged commit 584ecbb into saschatimme:master Nov 29, 2017
@saschatimme
Copy link
Owner

Do you have other changes planned or shall I cut an release?

@edennis
Copy link
Contributor Author

edennis commented Nov 29, 2017

@saschatimme Thanks! I haven't actually popped everything from the stack yet - there is one more coming, but I can't make any promises about ETA. I'd probably go ahead with a version bump now for the sake of granularity just in case anyone reports regressions.

@edennis edennis deleted the confused-results branch November 29, 2017 08:39
@markhamburg
Copy link

I pulled the code into my project and tried to recreate the situation we had before and it all worked correctly this time. The diff against my own patches also confirmed that this was doing what I expected.

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.

3 participants