Allow Compatible Subscribe Decorators To Contain Extra Data

[shellscape]

This PR proposes to add the ability for both the metadata explorer and the websocket controller to allow custom, compatible SubscribeMessage variant decorators additional metadata.

Targeted Problem

Both WebSocketsController and GatewayMetadataExplorer are locked into a single-use paradigm: they assume that a subscribe decorator will never provide more data than the message name (aka type). This means that decorators which seek to extend functionality are locked into an inflexible paradigm, making extending such decorators with additional metadata extremely difficult.

Use Case

We're currently developing a robust adapter for ws. The ws module differs from socket.io in several ways. One such way is the notable absence of "namespaces," which in ws can be handled simply by filtering and inspecting the pathname of a connected child WebSocket.

Consider the following decorator:

import 'reflect-metadata';
import { MESSAGE_MAPPING_METADATA, MESSAGE_METADATA } from '@nestjs/websockets/constants';
import { isObject, isUndefined } from '@nestjs/common/utils/shared.utils';
const MESSAGE_CHANNEL = 'channel';

export const SubscribeWsMessage = (
  message?: string,
  channel?: string,
): MethodDecorator => {
  let metadata = isObject(message) ? message.value : message;
  metadata = isUndefined(metadata) ? '' : metadata;

  return (target, key, descriptor: PropertyDescriptor) => {
    Reflect.defineMetadata(MESSAGE_MAPPING_METADATA, true, descriptor.value);
    Reflect.defineMetadata(MESSAGE_METADATA, metadata, descriptor.value);
    Reflect.defineMetadata(MESSAGE_CHANNEL, channel || 'default', descriptor.value);
    return descriptor;
  };
};

It's compatible with SubscribeMessage and adds additional channel data. This data will then be used by WsAdapter to filter child WebSocket connections, and subsequently only make calls to the associated callback(s) if the client sending a message happens to be on a particular channel.

The merits of our approach could surely be debated, there may even be better ways to go about this. However, the core issue remains - modifying the code to allow extra metadata to be passed along in handlers is a small cost for a large win for extensibility.

[shellscape]

Looks like there's an error with the new test added:

TSError: ⨯ Unable to compile TypeScript
src/websockets/test/gateway-metadata-explorer.spec.ts (77,23): Property 'extra' does not exist on type 'MessageMappingProperties'. (2339)
    at getOutput (/home/travis/build/nestjs/nest/node_modules/ts-node/src/index.ts:307:15)

Unfortunately I'm not sure how to fix this one, as my TypeScript skills are still in their infancy. I would guess that MessageMappingProperties in gateway-metadata-explorer.d.ts needs to be modified to accept any number of additional properties, but I'm not sure what the syntax for that should be.

[coveralls]

Coverage increased (+0.04%) to 93.916% when pulling 678bb65 on shellscape:subscribe-metadata into 7bb35c0 on nestjs:master.

[shellscape]

🎉 took a stab and it looks like it passes!

[kamilmysliwiec]

Hi @shellscape,
Thanks for your effort! Sorry for late response, I'm busy due to v5.0.0 release that contains a lot of new, powerful stuff, but as a result, requires lots of time.

The idea behind this pull request is truly understandable for sure. At the moment, I'm working on @websockets package to make this module much more flexible and customizable. This proposed improvement will be definitely considered as well 🙂 Please, give me some time :)

[shellscape]

@kamilmysliwiec I can certainly understand the time concerns. Please do understand that this is currently blocking our entire company from using Nest as it currently is. While the 5.0.0 release is no doubt very important, publishing a patch release with this improvement would be much appreciated, even if it is temporary with the changes coming down the pipe with 5.0.0.

[joshwiens]

Work issues aside, this particular path from one maintainer to another doesn't seem terribly useful as an individual consumer.

The current websockets implementation doesn't let you fully leverage adapters leaving two options for resolution, feature improvement or as you suggested entirely new module.

The former is benign to anyone using the existing major version range unless they explicitly create a new websockets module that leverages the ability to add additional channel data.

The latter on the other hand needs to be finished, then published & then stabilize before the work to update all of our existing services can even begin with all of that assuming the use case is covered in 5.x which means there is a very real chance we are back here having this discussion again two months from now.

[kamilmysliwiec]

Hmm.. @d3viant0ne @shellscape Just wondering. Why cannot you use just @SubscribeMessage({ channel: '', message: '' } as any)? Moreover, you could create a wrapper decorator:

export const SubscribeWsMessage = (channel: string, message: string) => SubscribeMessage({ channel, message } as any);

Maybe there're some obstacles that I'm not seeing at the moment(? thinking about clean temporary solution).

[shellscape]

This may be my inexperience with TypeScript, but doesn't this line

nest/src/websockets/utils/subscribe-message.decorator.ts

Line 9 in 7bb35c0

message?: { value: string } | string,

preclude that from being valid?

[kamilmysliwiec]

In this case, it is only additional typing added to increase developer experience. As far as I know, casting to any should solve all problems (of course, this is only a temporary workaround :))

[joshwiens]

I'm not a huge fan of allowing any but for the sake of finding a temp. middle ground, if it works great.

[kamilmysliwiec]

I hope that this workaround is fine so far @d3viant0ne @shellscape. Inspired by this pull request, I'm gonna introduce a small enhancement in the nearest release that should simplify using any other ws library and make it much less painful. Thanks @shellscape 🔥 Also, I hope you have not given up with this Ws module!

[shellscape]

thanks @kamilmysliwiec. haven't given up at all! just need to get it published.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.