adds reenqueue by name processor #9
Conversation
test/deadLetterProcessors.js
Outdated
| @@ -56,6 +56,30 @@ describe('Deadletter processors', function() { | |||
| }); | |||
| }); | |||
|
|
|||
| // eslint-disable-next-line | |||
There was a problem hiding this comment.
long story short, it's not needed.
long story long: i thought it was jasmine so I was using fdescribe. little did I know it was mocha (also these tests are FAST)
| @@ -0,0 +1,12 @@ | |||
| const deadletterActions = require('./../deadletterActions'); | |||
There was a problem hiding this comment.
minor reenqueueAllByName
There was a problem hiding this comment.
I thought about it but seemed misleading. so I removed the All although I can be argued eitherways. don't feel too strongly about it.
There was a problem hiding this comment.
nope, its a minor comment
azharkhan
force-pushed
the
akhan/adds-reenqueue-by-eventname
branch
from
0800848
to
336df0e
Compare
|
@earobinson - updated with less |
| if (event === content.event) { | ||
| return Q.when(deadletterActions.REENQUEUE_MESSAGE); | ||
| } else { | ||
| return Q.reject(new Error(`Required ${event} does not match ${message.content.event}`)); |
There was a problem hiding this comment.
not sure we need Q for these, should be able to use native promises
There was a problem hiding this comment.
while I agree with that and I'd use native as much as possible, but I'm not sure (maybe I'm unaware) of a Q.when() implementation in the Promise prototype.
There was a problem hiding this comment.
in this case the parameter is just a string so Promise.resolve() could work? i find the when method mysterious indeed
There was a problem hiding this comment.
since I'm not sure if a Promise.resolve would work, I'd suggest in the interest of consistency, we can go with the Q library.
however, I'm up to investigate the Promise.resolve as a separate issue so that we can switch all the deadletterProcessors to use native promises and remove the dependency on Q
#winwin
what do you think @iain8 ?
There was a problem hiding this comment.
@iain8 is correct Promise.resolve() would work, it's up to you how much you want to scout this.
There was a problem hiding this comment.
@earobinson @iain8 - I've created an issue for this #10 in order to properly investigate this.
since the Q implementation is in step with the rest of the library, I think investigating it is definitely beneficial to reduce unnecessary dependencies, however outside of the scope of this PR.
test/deadLetterProcessors.js
Outdated
| describe('Re-Enqueue by Name Processor', function() { | ||
|
|
||
| it('should return with a promise containing requeue all messages action when the event name matches', function() { | ||
| const testScope = this; |
There was a problem hiding this comment.
i don't feel that aliasing the scope is necessary for using it in these tests
| const Q = require('q'); | ||
|
|
||
| module.exports = function(event, message) { | ||
| const content = JSON.parse(message.content); |
There was a problem hiding this comment.
parse() will throw a cheeky error if the message content isn't valid JSON, will this be handled gracefully by the library?
There was a problem hiding this comment.
in this context iirc the message is guaranteed to be valid json, also this would just cause our commandline app to crash
azharkhan
force-pushed
the
akhan/adds-reenqueue-by-eventname
branch
from
336df0e
to
a2a9205
Compare
We need a way to process large number of deadletter events by filtering specific events.
This processor just takes an event name as a property and enqueues matching events.