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

Emit sentFrame and receivedFrame events from endpoint. #135

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

timmckenzie
Copy link

Want to be able to inspect frames as the come and go. Added an onEndpoint parameter to Agent.request() and EndPoint(). Needed onEndpoint because after constructor for EndPoint has returned some frames have already been sent, and would be missed by any event handler. Also added onSentFrame and onReceivedFrame parameters to Connection() constructor for same reason.

Refactored the constructor for EndPoint and Connection to take a config object instead of a list of parameters. The parameter list on the constructor was getting pretty long.

Added test for sentFrame, and receiveFrame. Updated existing tests to handle new constructor for EndPoint and Connection.

Want to be able to inspect frames as the come and go.  Added an onEndpoint parameter to Agent.request() and EndPoint().  Needed onEndpoint because after constructor for EndPoint has returned some frames have already been sent, and would be missed by any event handler.  Also added onSentFrame and onReceivedFrame parameters to Connection() constructor for same reason.

Refactored the constructor for EndPoint and Connection to take a config object instead of a list of parameters.  The parameter list on the constructor was getting pretty long.

Added test for sentFrame, and receiveFrame.  Updated existing tests to handle new constructor for EndPoint and Connection.

Conflicts:
	lib/http.js
@timmckenzie
Copy link
Author

I also updated the inline documentation. Is the wiki just a copy and paste of the code comments, or is there another system which automatically updates the wiki from code comments?

Also this pull request contains breaking changes (constructor to Endpoint and Connection). Should increment the Major version.

@nwgh
Copy link
Collaborator

nwgh commented Aug 11, 2015

I'm not convinced the breaking API changes need to happen - it's just as easy to add arguments (which will default to undefined) without breaking backwards compat (and the argument lists really aren't that long). Other than that, this seems pretty OK on the surface of it. I'll take a more thorough look once the breaking changes have been reverted.

As to the wiki/pages for this - those are generated from the inline comments. It's automated in that it's not a strict copy/paste, but it's manual in that someone has to go in and run the doc generation (and update the gh-pages branch). I'll get to that one of these days :)

@timmckenzie
Copy link
Author

I like having an options object because having a mix of optional and required options becomes a mess to handle. For example, an optional onSocket parameter was added to the constructor. We would have to use Endpoint(myLog, 'CLIENT', mySetting, undefined, myOnSocketFunc) to avoid confusion between onEndpoint and onSocket parameters.

What if we leave the required arguments be function parameters, and add an optional options object?

So it would end up looking like:

var options = {
   myOnSocket: function(socket){}
};
Endpoint(myLog, 'CLIENT', mySetting, options); // ok
Endpoint(myLog, 'CLIENT', mySetting); // ok

This would not break backwards compatibility, and allow for future expansion of arguments.

What do you think?

@nwgh
Copy link
Collaborator

nwgh commented Aug 12, 2015

Hrm... that feels even less desirable, to me - mixed metaphors in a way, with some arguments being explicit/positional, and some being in a (positional) dict. I'm tempted to go one of two routes here - either we just hold this patch back until more API breaking changes come along that we can batch this with (I would prefer to keep API churn to a minimum now that h2 is complete), or do the time-honored trick of looking at the first argument to the function, if it's a dict, treat it like the function was called in this new way (with an options dict and nothing else), and otherwise, treat it like it was called using the current API.

I'm leaning towards the latter, because I do see some value in having this functionality, though I also feel like there should be a better way to manage this entirely, in order to avoid synchronous callbacks. I'll have to think on it some more.

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

Successfully merging this pull request may close these issues.

2 participants