Rough STOMP Adapter

[aeberlin]

Purpose: Adds STOMP adapter + interfaces + documentation for use with Sync.

Changes:

• Exercise more built-in bundler functionality in .travis.yml and force install.
• Added / Updated ruby versions in .travis.yml
• Add rabbitmq service and configuration in .travis.yml (for STOMP)
• Add api_key, auth_token, websocket, destination, and debug to configuration.
• Allow debug to fall back as true for !production? when omitted from sync.yml.
• Allow Sync.onReady(..) to accept an array of callbacks.
• Ensure Sync.readyQueue callbacks do not fire until adapter is available and connected.
  • Start Sync inside self-closing interval (in case adapter dependency loading is deferred).
  • Defer Sync.onReady callbacks until adapter is connected (see onConnectInterval).
• Use api_key and auth_token for login and passcode for STOMP client authentication.
• Removed redundant subscribe(..) under Pusher adapter class.
• Rename PUSHER_API_KEY to simply API_KEY.
• Added Stomp adapter + subscription classes to sync.coffee.erb.
• Added Stomp client adapter in ruby.
• Added template specification for STOMP adapter to sync.yml.
• Added message test, test helper, and fixture for STOMP.
• Updated README.md with basic instructions for using the STOMP adapter with RabbitMQ.
• Force bundler gem update in .travis.yml.
  • Fix for issue bundler/bundler#3558.

[aeberlin]

@chrismccord Thoughts?

[chrismccord]

It would be nice if this could be done as an external adapter. Any thoughts on going that approach?

[aeberlin]

Hey, @chrismccord.

It shouldn't be too hard to extract it to it's own gem. I was actually going to suggest that we extract Faye, Pusher, and this adapter out to their own plugin projects after this iteration was complete.

I think, though, that we need to solidify and maybe refactor a couple of the interface patterns used throughout the gem before we go down that route. Here are some thoughts:

1. Sync::Clients: We should define a base interface that other client adapters inherit from, which throws NotImplementedError's when necessary to emphasize expected delegation to the child class. Doing so would establish a very clear set of expectations and directions for the development of other adapters. It would probably yield more reusable patterns for testing examples, too.

2. Sync.Adapter: The idea is generally the same as above, but as this structure is much more evolved than on the ruby side, I think maybe only a little bit of cleaning would be necessary here. Probably just a more strict set of expectations or documentation regarding callbacks and boot structure, and it'd be done.

3. Extract inline-rendered javascript snippets from the ruby files into templates.

4. The general file organization under lib/sync could do with some cleaning, and could probably make good use of ActiveSupport::Concern versus vanilla module composition. Quick sketch:

- lib/sync
  |- clients/
     |- concerns/
        |- faye_extension.rb
     |- base.rb
     |- dummy.rb
     |- faye.rb
     |- pusher.rb
     |- stomp.rb
  |- helpers/
     |- concerns/
     |- controller_helpers.rb
     |- model_actions.rb
     |- etc
  |- interactors/
     |- partial_creator.rb
     |- refetch_partial_creator.rb
     |- refetch_partial.rb
     |- renderer.rb
  |- trackers/
     |- base.rb
     |- erb.rb

This is very rough, and I won't speak to how accurate it is, but I think that it should be ironed out before trying to enable a conventions-based plugin architecture. Just a little bit of reorganization could go a long way.

5. I think that minitest is becoming a bit burdensome as a test framework for this project. I'm usually quite impartial with regards to choices here, but rspec-rails would really make the suite more flexible and reusable. Something like rubocop would help clean up the existing codebase style and enable people to more readily contribute. I would also recommend using FactoryGirl to DRY things up and reduce/remove reliance on fixtures.

Those are my most immediate general thoughts. I wouldn't mind working through some of these thoughts in a follow-up pull request or two, but I think it would be a bit much for me to take on by myself. I would like to see this PR get merged first so that it's integrations aren't lost in the mean time. After that, I'd love to collaborate on this, as I'm actively using this gem and kicking off it's usage in another project with this STOMP adapter.

Sorry to dump so many thoughts on you, but drop your two cents in when you have a chance.

It's definitely worth the time, so let me know. Cheers.

[aeberlin]

@chrismccord is there anything I can do in the short-term to help get this merged?

[aeberlin]

I understand there are some merge conflicts due to recent changes, but I'm still looking for general feedback on how to get this PR merged. @chrismccord or @ajb, feedback and advice for resolving merge conflicts, please?

Thanks, cheers.

[ajb]

Heya @aeberlin,

Sorry for the delay. I'm onboard with a lot of your ideas in the long comment above. In fact, I'd like to try and outline a rough plan for the next major version of sync, (which, by the way, will need to be renamed [#215]), but I haven't found the time to do so yet.

Anyway, I don't want to let that get in the way of this PR. This PR looks solid, and if you can resolve the merge conflicts, I can commit to taking the time to review & merge.

[aeberlin]

Great, thanks @ajb. Will start working through the conflicts.

[aeberlin]

@ajb Think I've resolved all the conflicts cleanly. Please review when you have a chance.

After looking over recent changes, I've noticed some similarities in the config keys for pusher and stomp, namely the websocket references. Do you have any thoughts on trying to generalize the pusher_-specific keys for more familiarity and flexibility across adapters? Not sure if we want to tackle that in this PR, but still curious.

Will check back after travis builds complete in case I missed something. Cheers.

[aeberlin]

@ajb Please restart this job when you get a chance, issue doesn't look related to the changes.

https://travis-ci.org/chrismccord/sync/jobs/108574596

[ajb]

Care to explain this a bit? I don't think we're setting any configs automatically based on the Rails environment, so it might be best to just leave this as:

def debug_flag
  config[:debug]
end

...unless I'm missing something?

[aeberlin]

Nah, artifact of a legacy app I was testing this in. 👍

[aeberlin]

You're not wrong, see my comment on the overall thread below. 👍

[ajb]

This looks great; just added a few inline comments above.

Can you comment on some of the changes around the initialization, onReadyInterval, onConnectInterval, etc? Were they necessary for STOMP, or just refactoring?

[aeberlin]

@ajb,

With regards to the coffee initialization process, I think I can simplify it by requiring that sync dependencies be loaded explicitly, without auto-loading themselves. If we can require a minor breaking change which enables sync to initialize itself explicitly, instead of on require, initializing itself and binding to window, etc., it will allow me to omit those changes. Thoughts?

[aeberlin]

Overall reasoning was because of a race condition with dependencies I experienced locally. Not a huge deal, but it made me look at things a bit differently.

[aeberlin]

@ajb I still need to root out some issues with the debug_flag and stompjs, but do you see anything lingering still that needs to be addressed?

[ajb]

Looks good! The only big thing remaining is the initialization process & intervals... you say there was a race condition that you were experiencing locally? Maybe I can look into that for you?

I would also be fine with this, if you think it will be an improvement:

With regards to the coffee initialization process, I think I can simplify it by requiring that sync dependencies be loaded explicitly, without auto-loading themselves. If we can require a minor breaking change which enables sync to initialize itself explicitly, instead of on require, initializing itself and binding to window, etc., it will allow me to omit those changes. Thoughts?

Since we'll be releasing 1.0.0 (under a different name, too), now would be the time to make breaking changes.

[aeberlin]

@ajb,

The race condition was due to order of dependencies in sprockets. I took a stab last night at removing the auto-initialization and found that it was more complicated to include via sprockets than I had thought before. Having said that, I think that using the intervals is the best way to go, as it allows the Sync coffeescript to initialize (and later connect) itself fully as soon as things are ready and available.

Thoughts?

[ajb]

I'm still in favor of removing the onDebug function completely, unless there's a reason it needs to be added.

[ajb]

I see a couple of potential issues with the current implementation of setInterval:

• setInterval(fn, 250) waits 250ms before calling fn, instead of calling it immediately. We want everything loaded as soon as possible.
• The onReadyInterval variable in view_helpers.rb is leaking into the global scope. We should wrap everything in an anonymous function, and use the var keyword.