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

Bind networkMonitor to _tasks. #3

Closed
wants to merge 3 commits into from

Conversation

@sathishsoundharajan
Copy link

sathishsoundharajan commented Nov 18, 2015

Fixes: #2

concurrents: this._networkMonitor.concurrents,
responseTimes: this._networkMonitor.responseTimes,
sockets: this._networkMonitor.sockets
requests: this._networkMonitor.requests.bind(this._networkMonitor),

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 18, 2015

Member

This better be done in NetworkMonitor itself

This comment has been minimized.

Copy link
@sathishsoundharajan

sathishsoundharajan Nov 18, 2015

Author

@Marsup How can i bind networkMoitor context for those func which is getting executed via Items.parallel.

Give me an idea, i will make appropriate changes.

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 18, 2015

Member

In the constructor.

This comment has been minimized.

Copy link
@arb

arb Nov 18, 2015

Contributor

I'm not sure about how to solve this the best way. In the Network constructor? this.requests = this.requests.bind(this)? That doesn't really make sense because the only reason the this pointer is out of whack is because the thing calling it is messing it up, Oppsy in this instance. Shouldn't the this pointer manipulation be done there instead?

This comment has been minimized.

Copy link
@sathishsoundharajan

sathishsoundharajan Nov 18, 2015

Author

@Marsup do you mean adding bind like this?
https://github.com/hapijs/oppsy/blob/master/lib/network.js#L21

Coz, i tried still the context is pointing to (_tasks).

If this not what you mean. please help me understand.

This comment has been minimized.

Copy link
@arb

arb Nov 18, 2015

Contributor

Or you can drop the prototypes entirely and just attach them to this in the constructor. That's probably a cleaner solution.

This comment has been minimized.

This comment has been minimized.

Copy link
@sathishsoundharajan

sathishsoundharajan Nov 18, 2015

Author

@arb, okie then i will move those methods to constructor and push it.

This comment has been minimized.

Copy link
@arb

arb Nov 18, 2015

Contributor

Is that going to be done in the next few minutes? I've already got a working branch locally and would like to close this ASAP since it basically doesn't work as is.

This comment has been minimized.

Copy link
@sathishsoundharajan

sathishsoundharajan Nov 19, 2015

Author

@arb Sorry, i couldn't make the changes. Thanks for the quick fix.

Sathish Kumar
@arb arb closed this Nov 18, 2015
@arb arb reopened this Nov 18, 2015
@arb arb added the bug label Nov 18, 2015
@arb arb added this to the 1.0.1 milestone Nov 18, 2015
@arb arb closed this Nov 18, 2015
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.