Skip to content
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

Refactor Broadcast allowing for customizable transport stream decorators #59

Open
shawncplus opened this issue Mar 2, 2019 · 5 comments
Labels
enhancement New feature or request
Milestone

Comments

@shawncplus
Copy link
Member

shawncplus commented Mar 2, 2019

The sty coloration library is currently hard coded inside the Broadcast class. Because Channel is a core class which uses Broadcast that makes core opinionated about how color should work. This makes it impossible to customize if you:

  • Don't want color
  • Don't like the sty syntax
  • Aren't using ANSI color

My high level concept is to create a system of customizable StreamDecorators that can be attached to TransportStreams in such a a way that a developer wouldn't have to modify core code AND wouldn't have to modify the transport bundle's (like telnet-networking code).

The difficulty is that TransportStream types aren't registered anywhere in the way that EntityLoaders are registered. This means there is currently no good central place to create the Stream+Decorators bindings.

Possible solutions

Bundle-centric

In this approach the transport bundle (e.g., telnet-networking) would entirely be in charge of this: they'd handle the initialization, customization, and management of decorators.

Pros

  • Maximum dev control

Cons

  • Little to no inter-bundle portability as every bundle might have a different configuration scheme, different decorator interfaces, etc.
  • All the work is foisted on the bundle dev
  • Likely to unintentionally imply that decorators should be hard coded into a transport bundle

Core-centric

In this approach there would be a base TransportDecorator class in core in the same way there is a base TransportStream class. Additionally the core would have to decide upon and own the configuration/binding strategy for Stream+Decorators

Pros

  • Easier on bundle developers
  • Easier for bundle users
  • Portable decorators
  • Unified configuration style

Cons

  • Less control for stream bundle developers since decorator configuration/binding will happen outside of their code
  • Having core do the configuration/binding means that the scheme will have to be more abstract and thereby potentially more complex than if it were up to the bundle itself

For me the core-centric approach seems like the obvious choice. The pieces of that puzzle look something like this:

TransportStream

TransportStream will need some way to identify themselves in such a way that configuration could link a transport stream to its decorators, my initial idea is that the class should get a new identifier getter, for example:

class TelnetStream extends TransportStream
{
  static get identifier() {
    return 'telnet';
  }

  // ...
}

TransportDecoratorRegistry

My idea for how to actually bind decorators to a stream involves creating a new TransportDecoratorRegistry. This class will be in charge of holding the configuration from ranvier.json which will look similar to DataLoader configuration:

// ranvier.json
{
  // register available decorators
  "transportDecorators": {
    "ansi": {
      // value follows same rules as DataLoader require config
      "require": "./lib/MyAnsiDecorator",
      // Serves as the default configuration for this decorator
      // which can be overridden by 'config' in the binding below
      "config": {
        "someDecoratorOption": 25
      }
    }
  },

  "transportDecoration": {
    "telnet": [
      {
        "decorator": "ansi",
        "config": {
          // additional configuration specific to this telnet:ansi binding
          "anotherOption": "foobar",
          // override of default config from registration
          "someDecoratorOption": 12,
        }
      }
    ]
  }
}

This is really verbose with all the configs but in practice it will look more like this:

{
  "streamDecorators": {
    "ansi": {
      "require": "cool-ansi-decorator"
    }
  },
  "streamDecoration": {
    "telnet": [
      { "decorator": "ansi" }
    ]
  }
}

To actually facilitate the usage of these decorators TransportDecoratorRegistry will have a decorate(TransportStream streamConstructor) method. This method will be used by transport bundle developers which means if a bundle developer so chooses they can not allow applying decorators:

class TransportDecoratorRegistry {
  decorate(streamConstructor) {
    const decorators = this.getDecorators(streamConstructor.identifier);
    return class extends streamConstructor {
      write(message, encoding) {
        message = decorators.reduce((acc, d) => d.decorate(acc), message);
        super.write(message, encoding);
      }
    };
  },
}

Writing a decorator

To write a new decorator, similar to DataLoader, there is no base class to extend. Instead you just need to follow the prescribed API:

const sty = require('sty');
class MyAnsiDecorator {
  // required configure(object config)
  configure(config = {}) {
    this.config = config;
  },

  // required decorate(string message)
  decorate(message) {
    return sty.parse(message);
  }
}

This class can either be an npm module or just a local file.

Changes for transport bundle devs

All that is in core. For the actual transport bundle developer they would write the following:

// inside their server-event script, e.g., server-events/telnet-server.js

// current:
const stream = new TelnetStream();
stream.attach(telnetSocket);

// new:
const stream = state.TransportDecoratorRegistry.decorate(TelnetStream);
stream.attach(telnetSocket);
@shawncplus shawncplus added the enhancement New feature or request label Mar 2, 2019
@shawncplus shawncplus changed the title Broadcast refactor allowing for customizable transport stream decorators Refactor Broadcast allowing for customizable transport stream decorators Mar 2, 2019
@ratacat
Copy link
Contributor

ratacat commented Mar 3, 2019

I am thinking the Core Centric approach sounds better too. One thing I'm not quite understanding is...
This would just allow someone to easily define different color libraries for different transport streams, but it wouldn't offer anything in terms of syntax in the areas. Such as..STY has color tags like syntax, and suppose a different library has an entirely different syntax such a ~GThisIsGreen...how would someone manage both syntaxes in the area definitions?

I'm guessing that that question is outside the scope of what you're looking at here, but I wanted to ask incase I'm not fully appreciating it!

Thanks!

@shawncplus
Copy link
Member Author

shawncplus commented Mar 3, 2019

Syntax wise the decorator defines the syntax. If you don't like the syntax your decorator uses you'd write a translating decorator that converts from one syntax to another and put it before the coloring decorator in the list.

For example you're using sty as the decorator which uses <red>red stuff</red> but you still wanted to use ~Rred stuff you would have to write a decorator which converted ~Rred stuff to <red>red stuff</red>. Or, to be honest, if you're already writing the code to convert ~R to <red></red> you've done most of the parsing heavy lifting so you may as well just turn that into your coloration library as well and drop sty

@shawncplus shawncplus added enhancement New feature or request and removed enhancement New feature or request labels Aug 22, 2019
@shawncplus shawncplus added this to the 3.2 milestone Aug 22, 2019
@Sakeran
Copy link
Contributor

Sakeran commented Sep 29, 2019

I have a branch implemeneting this feature here: https://github.com/Sakeran/core/tree/transport-decorators

TransportDecoratorRegistry has been implemented mostly to the specification laid out here. Broadcast has also been modified to work better with decorators, and to remove opinionation about color. To be more specific, I've currently made the following changes to my branch:

socket.write() convention

This one is probably more important than the decorators themselves. The current standard when calling a TransportStream's write method is to use the convention socket.write(message, encoding). This branch proposes a switch to the convention socket.write(message, options), where options is an object with arbitrary data.

Example:

socket.write("Hello!", { encoding: "utf8", wrap: 80 });

This gives us a few advantages:

  • The second argument is of more use to other kinds of TransportStreams that don't use encoding
  • Options can be set through the Broadcast API, allowing developers a more direct interface for things like OOB, websocket payloads, etc.
  • Decorators have access to the options object, and can read from and/or modify it as necessary.

I may be overqualifying this point a bit, but it is a powerful change. It also doesn't break either of the existing networking bundles like I thought it might (websockets doesn't use it, telnet falls back to utf8), so migration isn't too much of an issue for anyone using those.

TransportDecoratorRegistry

As stated above, TransportDecoratorRegistry is implemented using the 'core-centric' pattern outlined in the initial post. It will do most of the heavy lifting when the engine starts up, using the transportDecorators and transportDecoration config options to create a Map of decoration functions.

Decorating a TransportStream works as specified, with decorate(streamConstructor), and requires that the Stream have a static identifier getter. This works, though I can imagine a case where the server might want to assign one of several sets of decorators (Telnet might want one of nocolor, ansi, xterm, etc). I wonder if it might be better to use something like decorate(streamConstructor, identifier), rather than requiring the developer to create multiple TransportStreams.

Broadcast

I added a new static function: to. This works exactly like at, except that it uses the "options" convention and passes it into socket.write(). at, and by extension its derivatives, keeps the same API, but strips out the sty parsing and populates an options object with useColor and wrapWidth, so decorators can pick up on them if set.

(to was the best name I could think up at the time. I'm open to suggestions for a better one)

A fair number of Broadcast's utility functions use sty for coloration by default, and there isn't really a non-breaking way to refactor these. If it's very important to keep them around, they could just be deprecated and left as-is, or we could modify the output such that coloration is removed.

Either way, it's not hard move the affected utility functions to wherever StyDecorator ends up living, so anyone who wants the syntax can still make use of them there.

EventUtil

One nice thing about decorators is that input events no longer need special consideration for coloration to work. I just stripped out the sty parsing for now, but nothing in this module seems strictly necessary at this point, and is mostly still around so I didn't have to rework the example input-events.


I may be missing a few other places in core where sty syntax is used by default - the colorify option for Channels is something I haven't touched yet. Once something has been done about the Broadcast utilities, though, it should be safe to drop the sty dependency from core altogether.

@Sakeran
Copy link
Contributor

Sakeran commented Sep 29, 2019

Additionally, here's an example of what a TransportDecorator class would look like in practice. The decorate function accepts two additional arguments for config and options, but otherwise works the same.

https://gist.github.com/Sakeran/5a1b8781ce0eb3ac6ad00957f4f99999

@NuSkooler
Copy link

(note: I debated opening a separate ticket for this so please let me know if that would be more appropriate)

I'd like to suggest that the entire Broadcast class itself should (also?) be pluggable. I'm working on a project using Ranvier that will make use of "static" screens for some scenarios such as combat. When the player is one one of these screens I do not want Broadcast to simply dump data with socket.write(), but instead to do custom placement/drawing.
It looks like this would be mostly doable by simply using MyClass.say vs Broadcast.say etc., but there are some areas in core such as Channels in which that would not apply to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants