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

Global channel subscription #97

Merged
merged 12 commits into from
Jun 26, 2013
Merged

Global channel subscription #97

merged 12 commits into from
Jun 26, 2013

Conversation

aismail
Copy link
Contributor

@aismail aismail commented Jun 24, 2013

#96


The result of the function is the channel id of the newly created
channel because we might need to use it right away.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use the @param notation to explain each parameter's type, default value and usage

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok!

@ovidiuch
Copy link
Contributor

LGTM out of context, and it can be merged, but I have two observations

  • A lot of defensive programming. I get that methods need to be sane, but the entire infrastructure should be saner (high-level first, right?), making each method work by itself might create code duplication and a more vague overall image
  • Somewhat related, but I feel we're still introducing a bit of technical debt, even though the methods are nicely written. The logic feels like it branches out completely from any previous logic. Before we translated channels when we created them, and passed the exact channel id to widgets, now we pass aliasses to widgets as well and they translate internally. Just something to think about

…ed to check whether widget should be initialized or not. (#96)
@@ -302,6 +310,9 @@ define ['cs!mozaic_module', 'cs!pubsub'], (Module) ->

$el.addClass("widget-#{name}")

# We need to translate global channels into their true uid here
params.channels = channels_utils.translateGlobalChannels(params.channels)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skidding and @topliceanu - I had to move translation here, because widget_starter waits for the channels to be initialized (have data) before initializing the widget. Therefore, the widget will never know that it had GLOBAL channels in the first place! The only one to know will be widget_starter, which seems reasonable - it's the intermediate layer between asking for a widget to be initialized and the widget initialization.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! gg for making optional effort :)

aismail pushed a commit that referenced this pull request Jun 26, 2013
@aismail aismail merged commit 576a64d into master Jun 26, 2013
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