FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

added request.setTimeout support by brandonrobertz · Pull Request #80 · browserify/http-browserify · GitHub

/ http-browserify Public

added request.setTimeout support - #80

Open
brandonrobertz wants to merge 3 commits into
browserify:masterfrom
brandonrobertz:master
ConversationCommits (3)ChecksFiles changed
Open

added request.setTimeout support#80
brandonrobertz wants to merge 3 commits into
browserify:masterfrom
brandonrobertz:master

Conversation

brandonrobertz commented Jan 27, 2015

Copy link
Copy Markdown

This is a port of node's request http setTimeout method.

brandonrobertz mentioned this pull request Jan 27, 2015
Open
dcousens reviewed Jan 27, 2015
View reviewed changes
Comment thread lib/request.js Outdated

dcousens Jan 27, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Could be:

this.xhr.ontimeout = this.emit.bind(this, 'timeout')

Depends on whether you prefer anonymous closures though.

brandonrobertz Jan 27, 2015

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I pretty much ported directly from the node implementation, but used the xhr-provided timeout scaffolding. The this.once('timeout', callback) just adds a one-time event handler. It shouldn't call back immediately.

I'm gonna be doing a little more testing with your binding/emit code. It's a lot cleaner.

dcousens Jan 27, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Right, sorry. My mistake. LGTM otherwise :)

brandonrobertz commented Jan 27, 2015

Copy link
Copy Markdown
Author

I've tested fa18e1f with my target libs. Looking good.

dcousens commented Jan 27, 2015

Copy link
Copy Markdown
Member

Could you add a basic test for this?
On 27 Jan 2015 2:45 pm, "B Roberts" notifications@github.com wrote:

I've tested fa18e1f
fa18e1f
with my target libs.

—
Reply to this email directly or view it on GitHub
#80 (comment)
.

brandonrobertz commented Jan 27, 2015

Copy link
Copy Markdown
Author

Yeah, I'll work on something. It's complicated, because so much of this depends on XMLHttpRequest. But it's worth it, so I'll see what I can put together. The existing tests mock out window with stubs, so I think I'll have to take a similar route with xhr.

brandonrobertz commented Jan 28, 2015

Copy link
Copy Markdown
Author

I wrote two tests. One to make sure request.setTimeout sets the correct xhr properties and another that simulates xhr timeout activity during send.

brandonrobertz commented Jan 29, 2015

Copy link
Copy Markdown
Author

If nobody objects or has comments ... @substack ?

vvo commented Mar 6, 2015

Copy link
Copy Markdown

@substack any comments on this? thx

seidtgeist mentioned this pull request Jun 19, 2015
Merged

connor4312 commented Sep 24, 2016

Copy link
Copy Markdown

Would it be possible to get this merged?

vvo commented Sep 26, 2016

Copy link
Copy Markdown

I believe someone willing to do it would have to ask @substack on twitter for repository and maintainer rights on this project. If he feels like he will have the bandwidth and will to do so.

brandonrobertz commented Sep 28, 2016

Copy link
Copy Markdown
Author

I spoke with substack on IRC a while back about this and it turns out browserify is now using stream-http when you require('http'). So it looks like this codebase is deprecated?

vvo commented Sep 28, 2016

Copy link
Copy Markdown

So it looks like this codebase is deprecated?

Yes if you are using the latest browserify. Some people may not but not sure we need to fix this :)

Then the next step would be to deprecate this module officially in the README, close issues and PRs.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL