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

New interface #440

Merged
merged 3 commits into from Mar 1, 2016
Merged

New interface #440

merged 3 commits into from Mar 1, 2016

Conversation

@arb
Copy link
Contributor

arb commented Feb 29, 2016

Brand new reporter interface. In summary, it looks like this...

Reporters is an object. Each key is a reporter "name" and the value is an array. Inside the array, you can pass a string, an already instantiated stream object, or a configuration object.

If you pass a string, it's assumed to be a path or node module that can be required and results in a constructor function that can create new streams via new.

If if an already instantiated object, it needs to be some kind of stream.

Finally, if it's a configuration object it needs to have one key; ctor. In there, you can set up the module that should be required, name for instances where an imported module exports more than one thing, and args will be an array that gets passed into the constructor.

If the constructed stream has a start method, it will be called and supplied start arguments if there are any, otherwise, start will just receive a standard callback.

After all the streams are created, there are piped together using Pumpify. When events come in from the hapi server, a copy of the data payload is pushed through each pipeline to be processed by the array of streams.

@arb arb added this to the 7.0.0 milestone Feb 29, 2016
}
event.event = eventName;
this._dataStream.push(Object.freeze(event));
const args = (arguments.length === 1 ? [arguments[0]] : Array.apply(null, arguments));

This comment has been minimized.

Copy link
@arb

arb Feb 29, 2016

Author Contributor

Because the way hapi delivers payloads to regular event handlers differs from version to version, this is the only way I could think of to capture all the arguments that hapi sends and still work with version >= 10.x.x. I am open to suggestions here.

This comment has been minimized.

Copy link
@cjihrig

cjihrig Feb 29, 2016

Contributor

Am I missing something? This looks like the same thing in both conditions.

This comment has been minimized.

Copy link
@arb

arb Mar 1, 2016

Author Contributor

If there's only one argument, you don't have to do the expensive Array.apply thing.

This comment has been minimized.

Copy link
@cjihrig

cjihrig Mar 1, 2016

Contributor

Why not just do it the Babel way. Create an array and loop over arguments to populate it.

if (typeof item.reporter === 'string') {
item.reporter = require(item.reporter);
}
ctor = ctor.bind.apply(ctor, ctorArgs);

This comment has been minimized.

Copy link
@arb

arb Feb 29, 2016

Author Contributor

This whacky syntax is needed to create dynamic objects with dynamic arguments into the constructor. Some day when node supports ... this can go away. Till then, this is it.

This comment has been minimized.

Copy link
@Marsup

Marsup Mar 1, 2016

Member

It does but until hapi puts the baseline on node 5+...

@@ -33,7 +34,7 @@
"lab": "7.3.0"
},
"scripts": {
"test": "lab -m 5000 -t 100 -v -La code",
"test": "lab -m 5000 -t 100 -v -La code || node -e ''",

This comment has been minimized.

Copy link
@cjihrig

cjihrig Mar 1, 2016

Contributor

Won't this prevent the CI from knowing if the tests failed?


const pipeline = Pumpify.obj(randomid, stringify, process.stdout);

//dataStream.pipe(pipeline);

This comment has been minimized.

Copy link
@cjihrig

cjihrig Mar 1, 2016

Contributor

Commented code and a debugger statement in this file.

}
start(done) {

setTimeout(() => {

This comment has been minimized.

Copy link
@cjihrig

cjihrig Mar 1, 2016

Contributor

This can just be setTimeout(done, 10);

events: { log: '*' }
}]
reporters: {
'foo':[

This comment has been minimized.

Copy link
@cjihrig

cjihrig Mar 1, 2016

Contributor

Space after :

This comment has been minimized.

Copy link
@Marsup

Marsup Mar 1, 2016

Member

Also no quotes needed.

};

two.init = (stream, emitter, callback) => {
const Strng = Stringify;

This comment has been minimized.

Copy link
@cjihrig

cjihrig Mar 1, 2016

Contributor

Is this supposed to be Strng to avoid conflicting with String? Could you just call it lowercase stringify?

This comment has been minimized.

Copy link
@Marsup

Marsup Mar 1, 2016

Member

I might be blind but I don't see any use of Stringify nor Strng anywhere in the tests, should it even exist ?

server.connection({ host: 'localhost' });
const consoleError = console.error;
const events = [];
//server.connection({ host: 'localhost' });

This comment has been minimized.

Copy link
@cjihrig

cjihrig Mar 1, 2016

Contributor

Commented code

const res1 = out1.data;
const res2 = out2.data;

//console.log(res1);

This comment has been minimized.

Copy link
@cjihrig

cjihrig Mar 1, 2016

Contributor

Commented code


Items.serial(this.settings.reporters, (item, next) => {
let ctor = undefined;

This comment has been minimized.

Copy link
@Marsup

Marsup Mar 1, 2016

Member

Be consistent in undefined, put it or not but not both. (rather not imho)

// If it has a reporter constructor, then create a new one, otherwise, assume it is
// a valid pre-constructed reporter
if (item.reporter) {
const ctorArgs = ctorOpts.args ? ctorOpts.args.slice(0) : [];

This comment has been minimized.

Copy link
@Marsup

Marsup Mar 1, 2016

Member

AFAIK slice() === slice(0)

pipe: Joi.func().required(),
start: Joi.func()
}).unknown(),
Joi.object().keys({

This comment has been minimized.

Copy link
@Marsup

Marsup Mar 1, 2016

Member

Why use .keys when it's not used in the previous one ?


const one = new GoodReporter();
one.init = (stream, emitter, callback) => {
it(`callsback with an error if a there is an error in a reporter 'start' method`, (done) => {

This comment has been minimized.

Copy link
@Marsup

Marsup Mar 1, 2016

Member

Space between calls and back ?

@@ -144,71 +215,98 @@ describe('Monitor', () => {
});
});

it('validates the incoming reporter objects', (done) => {
it('validates the incomming stream object instances', (done) => {

This comment has been minimized.

Copy link
@Marsup

Marsup Mar 1, 2016

Member

Single m

@arb arb force-pushed the arb:new-interface branch 3 times, most recently from 3464797 to 62f0e6e Mar 1, 2016
@arb arb self-assigned this Mar 1, 2016
@arb arb force-pushed the arb:new-interface branch from 62f0e6e to b03490e Mar 1, 2016
arb added a commit that referenced this pull request Mar 1, 2016
New interface
@arb arb merged commit fcb291d into hapijs:v7.0.0 Mar 1, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arb arb deleted the arb:new-interface branch Mar 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.