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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Action Cable: client side 馃拝 #24039

Merged
merged 4 commits into from Mar 4, 2016
Merged

Action Cable: client side 馃拝 #24039

merged 4 commits into from Mar 4, 2016

Conversation

@javan
Copy link
Member

@javan javan commented Mar 4, 2016

  • Moves the default mount path to ActionCable::INTERNAL to ensure it stays synced with action_cable.js (via action_cable.coffee.erb)
  • Restores ActionCable#getConfig to its original, intended design for reading any action-cable-* meta tag
  • Minor internal refactoring for clarity and convenience

/cc @maclover7, @jeremy

@rails-bot
Copy link

@rails-bot rails-bot commented Mar 4, 2016

r? @senny

(@rails-bot has picked a reviewer for you, use r? to override)

@identifier = JSON.stringify(params)
extend(this, mixin)
@subscriptions.add(this)

This comment has been minimized.

@maclover7

maclover7 Mar 4, 2016
Member

Where is this logic performed elsewhere, I think we still want to do this? Could be missing something -- not super familiar with CoffeeScript :)

This comment has been minimized.

@javan

javan Mar 4, 2016
Author Member

Moved it to Subscriptions#create (see ~13 lines down in this PR). Before, Subscriptions#create constructed Subscription instances with a reference to @subscriptions and the subscription would then add itself to the collection. I'm not sure why I implemented that way originally. Now, Subscriptions#create makes a Subscription and then adds it to its collection.

In short: same end result, less confusing circular object passing.

This comment has been minimized.

@maclover7

maclover7 Mar 4, 2016
Member

Ahh, gotcha. 馃憤 for less circular logic. :)

@maclover7
Copy link
Member

@maclover7 maclover7 commented Mar 4, 2016

In general, 馃憤 for refactoring this a bit.

@jeremy
Copy link
Member

@jeremy jeremy commented Mar 4, 2016

鉂わ笍

jeremy added a commit that referenced this pull request Mar 4, 2016
@jeremy jeremy merged commit 2cafbd3 into rails:master Mar 4, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lifo

This comment has been minimized.

@javan I think ActionCable.Subscriptions#create should always return the Subscription object. We expect that in our apps as well.

This comment has been minimized.

Copy link
Member Author

@javan javan replied Mar 4, 2016

Wups, good catch. Fixed up in #24057.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants