{"action":"created","issue":{"url":"https://api.github.com/repos/chrismccord/sync/issues/212","labels_url":"https://api.github.com/repos/chrismccord/sync/issues/212/labels{/name}","comments_url":"https://api.github.com/repos/chrismccord/sync/issues/212/comments","events_url":"https://api.github.com/repos/chrismccord/sync/issues/212/events","html_url":"https://github.com/chrismccord/sync/pull/212","id":123996794,"number":212,"title":"Rough STOMP Adapter","user":{"login":"aeberlin","id":1169939,"avatar_url":"https://avatars.githubusercontent.com/u/1169939?v=3","gravatar_id":"","url":"https://api.github.com/users/aeberlin","html_url":"https://github.com/aeberlin","followers_url":"https://api.github.com/users/aeberlin/followers","following_url":"https://api.github.com/users/aeberlin/following{/other_user}","gists_url":"https://api.github.com/users/aeberlin/gists{/gist_id}","starred_url":"https://api.github.com/users/aeberlin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/aeberlin/subscriptions","organizations_url":"https://api.github.com/users/aeberlin/orgs","repos_url":"https://api.github.com/users/aeberlin/repos","events_url":"https://api.github.com/users/aeberlin/events{/privacy}","received_events_url":"https://api.github.com/users/aeberlin/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":3,"created_at":"2015-12-27T21:19:42Z","updated_at":"2015-12-28T20:45:47Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/chrismccord/sync/pulls/212","html_url":"https://github.com/chrismccord/sync/pull/212","diff_url":"https://github.com/chrismccord/sync/pull/212.diff","patch_url":"https://github.com/chrismccord/sync/pull/212.patch"},"body":"Purpose: Adds STOMP adapter + interfaces + documentation for use with Sync.\r\n\r\nChanges:\r\n- [X] Exercise more built-in bundler functionality in `.travis.yml` and force install.\r\n- [X] Added / Updated ruby versions in `.travis.yml` \r\n- [X] Add `rabbitmq` service and configuration in `.travis.yml` (for STOMP)\r\n- [X] Add `api_key`, `auth_token`, `websocket`, `destination`, and `debug` to configuration.\r\n- [X] Allow `debug` to fall back as true for `!production?` when omitted from `sync.yml`.\r\n- [X] Allow `Sync.onReady(..)` to accept an array of callbacks.\r\n- [X] Ensure `Sync.readyQueue` callbacks do not fire until adapter is available and connected.\r\n  - [X] Start Sync inside self-closing interval (in case adapter dependency loading is deferred).\r\n  - [X] Defer `Sync.onReady` callbacks until adapter is connected (see `onConnectInterval`).\r\n- [X] Use `api_key` and `auth_token` for `login` and `passcode` for STOMP client authentication.\r\n- [X] Removed redundant `subscribe(..)` under Pusher adapter class.\r\n- [X] Rename `PUSHER_API_KEY` to simply `API_KEY`.\r\n- [X] Added `Stomp` adapter + subscription classes to `sync.coffee.erb`.\r\n- [X] Added `Stomp` client adapter in ruby.\r\n- [X] Added template specification for STOMP adapter to `sync.yml`.\r\n- [X] Added message test, test helper, and fixture for STOMP.\r\n- [x] Updated `README.md` with basic instructions for using the STOMP adapter with RabbitMQ.\r\n- [X] Force `bundler` gem update in `.travis.yml`.\r\n  - Fix for issue [bundler/bundler#3558](https://github.com/bundler/bundler/issues/3558)."},"comment":{"url":"https://api.github.com/repos/chrismccord/sync/issues/comments/167650751","html_url":"https://github.com/chrismccord/sync/pull/212#issuecomment-167650751","issue_url":"https://api.github.com/repos/chrismccord/sync/issues/212","id":167650751,"user":{"login":"aeberlin","id":1169939,"avatar_url":"https://avatars.githubusercontent.com/u/1169939?v=3","gravatar_id":"","url":"https://api.github.com/users/aeberlin","html_url":"https://github.com/aeberlin","followers_url":"https://api.github.com/users/aeberlin/followers","following_url":"https://api.github.com/users/aeberlin/following{/other_user}","gists_url":"https://api.github.com/users/aeberlin/gists{/gist_id}","starred_url":"https://api.github.com/users/aeberlin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/aeberlin/subscriptions","organizations_url":"https://api.github.com/users/aeberlin/orgs","repos_url":"https://api.github.com/users/aeberlin/repos","events_url":"https://api.github.com/users/aeberlin/events{/privacy}","received_events_url":"https://api.github.com/users/aeberlin/received_events","type":"User","site_admin":false},"created_at":"2015-12-28T20:45:47Z","updated_at":"2015-12-28T20:45:47Z","body":"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.\r\n\r\nI 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:\r\n\r\n1) `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.\r\n\r\n2) `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.\r\n\r\n3) Extract inline-rendered javascript snippets from the ruby files into templates.\r\n\r\n4) 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:\r\n\r\n```\r\n- lib/sync\r\n  |- clients/\r\n     |- concerns/\r\n        |- faye_extension.rb\r\n     |- base.rb\r\n     |- dummy.rb\r\n     |- faye.rb\r\n     |- pusher.rb\r\n     |- stomp.rb\r\n  |- helpers/\r\n     |- concerns/\r\n     |- controller_helpers.rb\r\n     |- model_actions.rb\r\n     |- etc\r\n  |- interactors/\r\n     |- partial_creator.rb\r\n     |- refetch_partial_creator.rb\r\n     |- refetch_partial.rb\r\n     |- renderer.rb\r\n  |- trackers/\r\n     |- base.rb\r\n     |- erb.rb\r\n```\r\n\r\nThis 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.\r\n\r\n5) 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.\r\n\r\nThose 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.\r\n\r\nSorry to dump so many thoughts on you, but drop your two cents in when you have a chance.\r\n\r\nIt's definitely worth the time, so let me know. Cheers."}}