-
Notifications
You must be signed in to change notification settings - Fork 4
Feature: Support for namespaces #18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This is pehnomenal, @rgov! ❤️ Thank you so much for working on this. I will go through the code when I have a moment. Would love to have this merged. Namespaces support is a big missing chunk of SocketIO support. |
When mocking outgoing traffic with `emit()`, you can pass an object in the first argument to specify the namespace in addition to the event name: | ||
|
||
```js | ||
io.client.emit({ event: 'power_on', namespace: '/bedroom/ceiling_fan' }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this how namespace-bound events are also dispatched in SocketIO normally? We should try to mimic that API to benefit from developer familiarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the Socket.IO Client API, you simply connect to scheme://host[:port][/namespace]
and then emit to the returned socket object like any other. I don't think we can mimic that.
In Server API, there is server.of()
. You would write io.client.of('/bedroom/ceiling_fan').emit('power_on')
.
If we did add an SocketIoConnection.of()
method that returned some Namespace
instance with on()
and emit()
methods, it could lead to messy code because this object is now divorced from the client
or server
label: nsp.emit('foo')
no longer tells you which direction the event is being emitted.
Personally I would think it's not important to mimic the Socket.IO API. The binding is already inconsistent with Socket.IO in how .on()
and .emit()
behave—e.g., the .on()
callback receives different arguments, and .emit()
works in the reverse direction. Using the same names with different semantics actually increases developer friction. Socket.IO's Server and Client APIs also aren't always identical so for each feature there would be an arbitrary decision of which API to mimic.
.of()
is just a terrible method name anyway. It doesn't seem to relate to the concept of namespaces at all, and the sequence .of().emit()
is meaningless to the reader as a phrase.
```js | ||
interceptor.on('connection', (connection) => { | ||
const io = toSocketIo(connection) | ||
io.setAuthorizer((namespace) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is .setAuthorizer
a thing in SocketIO normally? If not, I'd probably vote against such a convenience wrapper and recommend people to implement their own. Will also take one line of code but give us fewer APIs to maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a Socket.IO server you have use()
which defines middleware that can act as an authorizer. This is a more focused feature.
The mock needs to respond to the client's CONNECT
request. It does so before my changes as well, but without supporting namespaces. If the mock is going to respond to CONNECT
, the developer should be able to override whether the connection succeeds or fails. That's all the idea is.
If you removed the overridable authorizer behavior, you could accept connections on any namespace, but then you cannot fully mock the server in some situations—such as confirming the isolation of per-user namespaces, or requiring a JWT token before the backend starts streaming sensitive data, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some complexity mimicking Socket.IO's API. How they intend it is:
// deny all connections on all namespaces
io.on("new_namespace", (namespace) => {
namespace.use((socket, next) => {
next(new Error("Not authorized"))
})
})
The middleware callback does not get passed the namespace name at invocation time, and it's not an attribute of the socket
instance. Recreating this machinery seems like it's not worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option I considered is io.client.onConnect((namespace: string) => bool)
— this mirrors the semantics of io.client.on()
being a way to capture what the client is doing. But unlike .on()
you can only have a single connection handler that allows or rejects the connection. And the Socket.IO Client API style would be io.client.on("connect", () => void)
(with no way to reject the connection at this point).
event: MessageEvent, | ||
details: SocketIoMessageDetails, | ||
): SocketIoMessageEvent { | ||
return new Proxy(event, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think if we instead created a subclass of MessageEvent
, providing the same socket-specific details on it without relying on a proxy?
class SocketIoMessageEvent extends Message Event {
public namespace: string
constructor(name, init: { namespace }) {
this.namespace = namespace
}
}
- Can ditch
event.socketio[xyz]
nesting as we are in the SocketIO context already; - Don't rely on a
Proxy
. - Have a more common custom event implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part of the implementation was suggested by a coding agent 😅 Since the MessageEvent
is an immutable object that's defined in some browser spec I didn't know how sound it is to try to copy and mutate it. Wouldn't we have to enumerate all of the properties and copy them from the object we're wrapping? How does that affect bubbling behavior? Etc...
I definitely defer to someone more experienced in how best to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The event instance should also carry socketio.event
.
) {} | ||
|
||
public on(event: string, listener: BoundMessageListener): void { | ||
public _onSocketIoPacket( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is meant as an internal method, I recommend using #
for the method name instead. That would truly make it private:
public _onSocketIoPacket( | |
#onSocketIoPacket( |
The method could then be used as this.#onSocketIoPacket()
within this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I struggled with this because SocketIoDuplexConnection.setAuthorizer()
has to use SocketIoConnection._onSocketIoPacket()
.
We could move the connection acceptance logic to SocketIoConnection
as in io.client.setAuthorizer()
(client? 😒) and then make _onSocketIoPacket()
private. Then sendMockEngineIoOpen
etc. could probably also move over to SocketIoConnection
which seems conceptually better.
public emit(event: string, ...data: Array<any>): void { | ||
public emit(event: string, ...data: Array<any>): void | ||
public emit(envelope: EventEnvelope, ...data: Array<any>): void | ||
public emit(envelope: string | EventEnvelope, ...data: Array<any>): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When designing this API, we really have to make sure we're mimicking how namespace-bound events are emitted in SocketIO normally. The closer we are, the better DX we would provide to developers mocking such connections.
this.server = new SocketIoConnection(this.rawServer) | ||
} | ||
|
||
private hasUpstreamServer(): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A nitpick: we're only using this method once. I'd probably vote for keeping the previous inline implementation unless we heavily rely on the upstream server checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I factored it out while attempting to implement a feature where the mock authorizer function could allow the connection through, which would be forwarded to the actual server, and then the actual server would respond to the CONNECT
message. This would allow mocking rejection behaviors even in the presence of a server that would normally accept. It would require using this logic in two places.
It’s clearer to read as a simple, expressive predicate if (hasUpstreamServer())
than to explain in a multi-line comment the esoteric behavior that accessing .readyState
before connecting will throw. The implementation also relies on the obscure Reflect.get()
, and catch
is being used for branching control flow, not error handling as many developers would expect.
So this particular check is especially well-suited to being pulled into a dedicated predicate function, which isolates all these elements of complexity and makes the logic of the call site more straightforward to understand.
}) | ||
} | ||
|
||
private sendMockSocketIoConnect(namespace: string): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe these should be methods. Instead, we can make them into simple functions that produce respective messages and then pass them to .send()
.
this.rawClient.send(messages.connect())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you envision the messages
object? A challenge is that the encoded packet is produced asynchronously:
encodePayload([openPacket], (encodedPayload) => {
this.rawClient.send(encodedPayload)
})
Are you thinking we would write:
MessageBuilder.connect(namespace, (encodedPayload) => {
this.rawClient.send(encodedPayload)
})
|
||
export function createSocketClient(uri: string): Socket { | ||
return io(uri, { transports: ['websocket'] }) | ||
return io(uri, { transports: ['websocket'], forceNew: true }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does forceNew
do? 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because our tests now create sockets on multiple namespaces, Engine.IO will multiplex them across a single WebSocket. This means we're leaking state between test cases, and only one test's interceptor connection handler will fire. Resolve this by disabling shared WebSockets.
I could add a comment to this effect if desired.
@kettanaito Any further thoughts about the API changes requested? I wrote my thoughts on the challenges making it match the Socket.IO API, which would introduce several contradictions or issues. |
This implements support for namespaces, an important part of Socket.IO. With these changes, it is now possible to (i) examine the namespace on which an intercepted event was sent, and (ii) inject mocked messages on a specific namespace. Fixes #17.
This required a few changes, each of which is a relatively small step. Reviewing this pull request commit-by-commit might be easier to understand the progression.
We need to listen to the client's
CONNECT
packet to see which namespace it is connecting to. I started by refactoring the code that listens toEVENT
packets so that I could reuse the deserialization logic to also supportCONNECT
.I added the ability to attach an authorizer callback to
SocketIoDuplexConnection
. This callback receives the namespace of the requested connection, and returns true if the connection should be allowed. This sends theCONNECT
orCONNECT_ERROR
ack packet.By default, the authorizer callback accepts any namespace. This is backwards-compatible and also matches how the real Socket.IO server behaves without defining any additional middleware.
Currently, the authorizer function is only used if there is no actual server connection. I contemplated giving the mock the ability to reject authorization or delegate to the actual server to complete the connection. However, it ended up being more complicated than it was worth—the intercepted WebSocket's
message
event listener would have to know to blockCONNECT
messages from the client.I also refactored the ack packets to use the imported Socket.IO serializer methods, rather than re-implement the protocol. However, it feels ugly to do this in
SocketIoDuplexConnection
and it should probably be cleaned up.(Actually, before 2.) Because our tests now create sockets on multiple namespaces, Engine.IO will multiplex them across a single WebSocket. This means we're leaking state between test cases, and only one test's interceptor connection handler will fire. Resolve this by disabling shared WebSockets.
When we intercept an event with
io.client.on()
, we would like to know the namespace it arrived on. This is a challenge, because the existing callback signature is(event: MessageEvent, ...data: any)
. If we add a newnamespace
argument, it won't be backward compatible.I solved this by extending the
MessageEvent
interface to add asocketio
property with{ namespace: string }
. SinceMessageEvent
objects are immutable, I had to wrap the original event in a proxy.Finally, we want to be able to emit mocked events that specify a namespace. Again, the signature
emit(event: string, ...data: any)
is problematic; there's nowhere to put anamespace
argument.I changed the
event
parameter toenvelope: { event: string, namespace?: string } | string
. If an object is passed, it looks for the namespace field or falls back to the default/
.While TypeScript/JavaScript are not my usual languages, I've aimed to make this implementation clear and correct. However, I'm happy for maintainers to refine it further.