Add the topic to the callback method before executing the callback #55
Conversation
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.
| * [`context`]: What `this` will be when the callback is invoked. | ||
| * `callback`: Function to invoke when the message is published. | ||
| * [`priority`]: Priority relative to other subscriptions for the same message. Lower values have higher priority. Default is 10. | ||
|
|
||
| > Returning `false` from a subscription will prevent any additional subscriptions from being invoked and will cause `amplify.publish` to return `false`. | ||
|
|
||
| You can find out which topic is currently being processed by accessing the `topic` property on your callback method. This is especially helpful when dealing with multiple topics in a single subscription. *Important: `topic` is only available for the duration of the callback function’s execution. If you defer/throttle or otherwise delay accessing the topic, it may no longer be the correct topic.* The easiest way to access this property is to use a named callback method. |
There was a problem hiding this comment.
s/processed/published/
There was a problem hiding this comment.
We don't use ’ anywhere else; should probably just use a regular quote.
There was a problem hiding this comment.
Changes made as requested. Good point about the ’ it was my obsessive compulsive nature kicking in!
|
Other than the two tiny readme comments, this looks good to me. I'll leave it open for a bit to allow discussion. |
|
Glad to see this going in. I think this is a win for amplify users! |
|
+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. |
|
@RedWolves I like that idea, probably more than the one in this PR. |
|
I am in favor of that method as well. Should it be |
|
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. I vote for |
|
I'm fine with either. |
|
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? |
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.