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

Updated to data stream and pipe method for transmitting data. #322

Merged
merged 2 commits into from Apr 3, 2015

Conversation

@arb
Copy link
Contributor

arb commented Apr 3, 2015

Closes #300

@arb arb added the breaking changes label Apr 3, 2015
@arb arb added this to the 6.0.0 milestone Apr 3, 2015
@arb arb assigned geek Apr 3, 2015
@arb arb force-pushed the arb:stream-event-reporter-interface branch from 541020f to 31c92d6 Apr 3, 2015
var Hoek = require('hoek');
var Items = require('items');
var Os = require('os');

This comment has been minimized.

Copy link
@geek

geek Apr 3, 2015

Member

Os was before Hoek to include node core modules first, then 3rd party, then local. This needs to be added to the styleguide.

var Hoek = require('hoek');
var Items = require('items');
var Os = require('os');
var Readable = require('stream').Readable;

This comment has been minimized.

Copy link
@geek

geek Apr 3, 2015

Member

In this case we wouldn't need to create a shortcut like this. Look at https://github.com/hapijs/hapi/blob/master/lib/response.js#L541

@@ -115,9 +119,8 @@ internals.Monitor.prototype.start = function (callback) {
};
};

for (var i = 0, il = this.settings.reporters.length; i < il; i++) {
Items.serial(this.settings.reporters, function (item, next) {

This comment has been minimized.

Copy link
@geek

geek Apr 3, 2015

Member

why not for ?

This comment has been minimized.

Copy link
@arb

arb Apr 3, 2015

Author Contributor

Because the reporter init function takes a callback.

item.args.unshift(null);
var constructor = item.reporter.bind.apply(item.reporter, item.args);
reporter = new constructor();
reporter = new item.reporter(item.events, item.config);

This comment has been minimized.

Copy link
@geek

geek Apr 3, 2015

Member

whats with the lowercase constructor?

emitter.once('stop', function () {

this.stopped = true;
}.bind(this));

This comment has been minimized.

Copy link
@geek

geek Apr 3, 2015

Member

could avoid the bind with a local self variable

var Hoek = require('hoek');
var Lab = require('lab');
var Http = require('http');

This comment has been minimized.

Copy link
@geek

geek Apr 3, 2015

Member

The original order was better with http first

geek added a commit that referenced this pull request Apr 3, 2015
Updated to data stream and pipe method for transmitting data.
@geek geek merged commit f372719 into hapijs:master Apr 3, 2015
@arb arb deleted the arb:stream-event-reporter-interface branch Apr 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.