socketio / socket.io-adapter

The Socket.IO in-memory adapter
https://socket.io/
197 stars 101 forks source link

serverSideEmit should be allowed #75

Closed lkho closed 2 years ago

lkho commented 3 years ago

I think the default adapter should either loopback or ignore (if the event is not intended to be received locally) the emit, instead of throwing. This makes the default adapter incompatible with other real cluster mode adapters and cannot be used as a drop in replacement in tests.

https://github.com/socketio/socket.io-adapter/blob/d4ef54cccb7e4a1b89fd4cdb46eefd192c69b5b8/lib/index.ts#L274-L278

darrachequesne commented 3 years ago

Hi! That's an interesting point. However, if I'm not mistaken the other adapters do not loopback on the given server (when serverSideEmit is used on serverA, it gets emitted on serverB and serverC, but not on serverA).

So we could probably ignore it here, but in that case, does it make sense to run the tests with this adapter?

lkho commented 3 years ago

it's fine to just ignore the events, but just don't throw errors. IMHO, this adapter should be a drop in replacement with other adapters (and vice versa), and should behave the same in user code. if it throws, then the user code need to check explicitly what the underlying implementation is.

In order to catch that error, I need to write some hacky code like this:

  // work around default adapter will throw error. fallback to local emit
  let serverSideEmitNotSupported = false;
  const tryServerSideEmit = (ev, ...args) => {
    try {
      if (serverSideEmitNotSupported) {
        // no need to loop back in this case
        // nsp._onServerSideEmit([ev, ...args]);
      } else {
        nsp.serverSideEmit(ev, ...args);
      }
    } catch (e) {
      if (e.message.indexOf('serverSideEmit()') >= 0) {
        serverSideEmitNotSupported = true;
        return tryServerSideEmit(ev, ...args);
      }
      throw e;
    }
  };
lkho commented 3 years ago

As for tests, in my deployment environment, I have a production (with redis) and UAT (no redis, single node process) server. I need to use the above hacky code to make it work when deployed on both servers.

darrachequesne commented 2 years ago

This should be fixed by https://github.com/socketio/socket.io-adapter/commit/808c0450d858bb0abc7d93c2b650ad5a2a20dda8, included in socket.io-adapter@2.4.0 (and socket.io@4.5.0).

Please reopen if needed.

MatanYemini commented 1 year ago

this adapter does not support the serverSideEmit() functionality

Still get's it. Version 2.5.2

darrachequesne commented 1 year ago

@MatanYemini you mean, the warning in the console? In that case, this is expected, as it warns users that calling serverSideEmit() is no-op with the in-memory adapter.