krakenjs / spud

A content store parser, reading a java .properties-like format
Other
14 stars 9 forks source link

SPUD Serializer ReadStream 'close' event handler #7

Closed kkleidal closed 10 years ago

kkleidal commented 10 years ago

The 'close' event set on the readstream was not firing, so I switched it to the end event. I also corrected its callback method to end the writeStream and access the correct property name ( writeStream['_data'] instead of writeStream['data'], which was undefined ).

The mocha unit tests all passed after my change, so I do not think the issue was specific to the custom serializer I am using in my project.

aredridel commented 10 years ago

Looks good to me, though will be obsoleted by the refactor we have pending.

You may want to look at that branch to see the changes coming soon; the interface to serializers is a little less magical, so it will probably affect you.

What sort of serializer are you writing?

kkleidal commented 10 years ago

It's very basic. I tried to follow the format on the Readme. It shouldn't be hard to change after the refactor. When are you planning on merging the refactor?

Here's my serializer so far. Pretty simple and easy to change after the refactor:

function MyReader() { }

    MyReader.prototype = {
        _doDeserialize: function( input, callback ) {
            setImmediate( function() {
                var data;
                try {
                    data = propHelper.parse( input );
                } catch ( e ) {
                    callback( e );
                    return;
                }
                callback(null, data);
            } );
        }
    };

    function MyWriter() {

    }

    MyWriter.prototype = {
        _doCreateReadStream: function (data) {
            return new PropReader(data);
        }
    };

    function PropReader(data) {
        Readable.call(this);
        this._finished = false;
        this._data = data;
    }
    util.inherits(PropReader, Readable);

    PropReader.prototype._read = function() {
        var i = this._index++;
        if ( this._finished ) {
            this.push(null);
        } else {
            this.emit()
            this.push( propHelper.serialize( this._data ) /* (a string) */ );
            this._finished = true;
        }
    };
aredridel commented 10 years ago

Neat! Looks straightforward. We should be merging it soon -- just doing a little code review.

What'll change is shown in the refactor branch's readme; inheritance is up to to the implementor of the serializer rather than the magic auto-inheriting when the serializer is registered.

kkleidal commented 10 years ago

closed because it will become obsolete after the refactor