@uppy/companion: upgrade redis to version 4.x

[aduh95]

Related to the socket.io upgrade. The new redis version is now using promises, which makes it a bit harder to interact with it from code that expects it to be synchronous. Fortunately, we were already using an util class to abstract that away, so I have hidden the promise interop boilerplate inside of it.

[mifi]

Nice going. Left some feedback.

• I have no problems with ditching node v10 but I think it would require a new major of companion, no?
• Shouldn't we wrap all the error, pSubscribe, pUnsubscribe events in a node EventEmitter instead, that way we can be compliant with the interface we're trying to simulate, and it would allow multiple .on(), once() etc listeners to be attached to a single event as is expected, and if someone calls removeListener('x', handler) it will only remove that specific handler, and not all other handlers too?

[mifi]

Will this work? Shouldn't message be JSON.stringified?

[aduh95]

• it would allow multiple .on(), once() etc listeners to be attached to a single event as is expected, and if someone calls removeListener('x', handler) it will only remove that specific handler, and not all other handlers too?

Isn't that already the case? We are already relying on RedisClient which I assume is an EventEmitter already. The error handler is a special case because that's what nrp is doing, although I suppose we don't have to stick to it if this is a breaking change anyway.

[mifi]

Isn't that already the case? We are already relying on RedisClient which I assume is an EventEmitter already. The error handler is a special case because that's what nrp is doing, although I suppose we don't have to stick to it if this is a breaking change anyway.

as i understand it, in the current implementation if someone calls:

const handler1 = (message) => console.log('listener1');
const handler2 = (message) => console.log('listener2');
emitter.on('some-event', handler1);
emitter.on('some-event', handler2);

emitter.removeListener('some-event', handler1)

then if some-event is received from redis, handler2 will never be called, although if it was a real event emitter, it would still call handler2 even though handler1 has been removed.

[mifi]

Actually looking at node-redis source code it does indeed seem to handle unsubscribing a specific listener already, if we pass it along. So if we change the removeListener implementation to pass the listener along to redis pubsubscribe, it should just work:

uppy/packages/@uppy/companion/src/server/emitter/redis-emitter.js

Line 85 in c17b2c4

connectedPromise.then(() => subscriber.pUnsubscribe(`${prefix}${eventName}`)).catch(errorHandler)

[mifi]

i finished the implementation and tested and it seems to be working nicely

[mifi]

ohh, are we still supporting node14 in uppy v3? It would be nice if we could support only 16 onwards, as we can use more built in features

[aduh95]

ohh, are we still supporting node14 in uppy v3? It would be nice if we could support only 16 onwards, as we can use more built in features

We are supporting Node.js 14.x in Companion, we are dropping support for EOL versions of Node.js (10.x and 12.x). We can discuss dropping support for 14.x, but it won't be EOL until next April. Marking as draft to ensure this doesn't land without discussing that first.

[Murderlon]

I think we should definitely comply with supported LTS versions, so in my opinion we shouldn't drop 14.x for some syntactic sugar.