search

mikehostetler / amplify

AmplifyJS
http://amplifyjs.com
GNU General Public License v2.0
1.45k stars 144 forks source link

Add the topic to the callback method before executing the callback #55

Open dcneiner opened 12 years ago

dcneiner commented 12 years ago

This allows the developer to access the current topic being handled by the callback without breaking existing implementations of amplify pub/sub. The example in the readme shows how to access this new property.

Also update the readme to clarify that multiple topics can be subscribed in a single subscribe call.

scottgonzalez commented 12 years ago

Other than the two tiny readme comments, this looks good to me. I'll leave it open for a bit to allow discussion.

ifandelse commented 12 years ago

Glad to see this going in. I think this is a win for amplify users!

RedWolves commented 12 years ago

+1 Doug and I had a discussion about this earlier today. My suggestion was to add topic to amplify so amplify.topic would be how you'd access it.

scottgonzalez commented 12 years ago

@RedWolves I like that idea, probably more than the one in this PR.

dcneiner commented 12 years ago

I am in favor of that method as well. Should it be amplify.topic or amplify.currentTopic? Also, @RedWolves do you want to submit a separate pull request – or should I update this one to reflect using amplify instead of the callback?

dcneiner commented 12 years ago

To clarify, I think using a global variable like this emphasizes the transitory nature of being able to access it. "If you don't reference it during the callback, it won't be there when you need it". I think that would be easier to understand with this approach vs. local callback variables.

RedWolves commented 12 years ago

Yeah I'll work on a pull request tonight and submit it. do we want to do .topic or .currentTopic?

dcneiner commented 12 years ago

I like both for different reasons. topic is shorter. currentTopic might be clearer.

I vote for topic.

scottgonzalez commented 12 years ago

I'm fine with either.

jdsharp commented 12 years ago

I'm not a big fan of amplify.topic. It doesn't sit well with me that you'd have to access an API outside of the current context to determine context. The topic is very specific to the handler being triggered and should be associated as much. (Unless I'm misunderstanding)

RedWolves commented 12 years ago

I put this together the other night https://gist.github.com/8af1ca8a678bcbe4e19c I have to agree with JD ... now that I see it implemented I am not a fan of the approach. Personally, I am still in favor of my first pull request (#47) with attaching topic to the end of args. Seems like any issues that arise could be defeated with documentation.

jakerella commented 10 years ago

Since this PR is well out of date, and given theat we no longer have docs in the README, I'm inclined to port this one-line change to a new PR, with the test, and open a separate issue on the documentation site.

Any concerns there @dcneiner?