Open shawncplus opened 5 years ago
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!
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
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:
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:
Broadcast
API, allowing developers a more direct interface for things like OOB, websocket payloads, etc.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.
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.
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.
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.
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
(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.
The
sty
coloration library is currently hard coded inside theBroadcast
class. BecauseChannel
is a core class which usesBroadcast
that makescore
opinionated about how color should work. This makes it impossible to customize if you:sty
syntaxMy high level concept is to create a system of customizable
StreamDecorators
that can be attached toTransportStreams
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 (liketelnet-networking
code).The difficulty is that
TransportStream
types aren't registered anywhere in the way thatEntityLoaders
are registered. This means there is currently no good central place to create theStream+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
Cons
Core-centric
In this approach there would be a base
TransportDecorator
class in core in the same way there is a baseTransportStream
class. Additionally the core would have to decide upon and own the configuration/binding strategy for Stream+DecoratorsPros
Cons
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: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 fromranvier.json
which will look similar toDataLoader
configuration:This is really verbose with all the configs but in practice it will look more like this:
To actually facilitate the usage of these decorators
TransportDecoratorRegistry
will have adecorate(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: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:
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: