amplify
amplify copied to clipboard
mikehostetler
Add the topic to the callback method before executing the callback
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.
Other than the two tiny readme comments, this looks good to me. I'll leave it open for a bit to allow discussion.
+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.
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?
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.
Yeah I'll work on a pull request tonight and submit it. do we want to do .topic or .currentTopic?
I like both for different reasons. topic is shorter. currentTopic might be clearer.
I vote for topic.
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)
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.
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?
