Request Next

[simov]

:christmas_tree: :sparkles: IT IS HAPPENING :gift: :tada:

[simov]

@mikeal @simov @nylen @seanstrom @isaacs @FredKSchott @greenkeeperio-bot @eiriksm @Turbo87 @froatsnook @papandreou @tikotzky @mmalecki @lalitkapoor @csainty @ahmadnassri @samcday @RReverser @jhs @twilson63 @joaojeronimo @aesopwolf @charlespwd @mscdex @kevinoid @benatkin @fantapop @janjongboom @netpoetica @0x4139 @rwky @emkay @wprl @mafintosh @aaron-em @guimonz @tbuchok @noway421 @nlf @thejh @Stanley @ZJONSSON @diversario @mgenereu @Filirom1 @DullReferenceException @dai-shi @hypesystem @watson @lexander @polotek @mitsuru @stash @lyuzashi @hueniverse @pvoznenko @VRMink @joshux @drudge @vmattos @apoco @poislagarde @mbrevoort @Marsup @alexindigo @dotcypress @mridgway @joemccann @cappslock @bcoe @shimaore @ReadmeCritic @crocket @clement @JoshWillik @LoicMahieu @hyjin @aj0strow @calibr @timshadel @jplock @vohof @notmatt @akshayp @davidlehn @tgohn @SamPlacette @zephrax @mikermcneil @YasharF @Cauldrath @jhurliman @goatslacker @itay @kevinburke @substack @jrgm @daniel347x @richarddong @jongyoonlee :100:

[froatsnook]

Congrats @simov. That's an impressive piece of work!

[ahmadnassri]

:+1: :+1: :+1: this is the perfect Christmas gift! will help in any way I can!

[mikeal]

Looks great!

Although I do wonder why the tests are moving to mocha when everyone seems to be leaving mocha.

[mikeal]

Oh, and one other question, do you have a target Node version you're working towards? Everything I work now is v4+ which is quite nice, I've gotten used to string templates everywhere :)

[mikeal]

oh, and a quick note on code style, it sounds like you are basically using standard. perhaps just move to standard entirely so that we can use the formatter?

[simov]

Although I do wonder why the tests are moving to mocha when everyone seems to be leaving mocha.

I'm probably missing the trends here. My point was that I felt ever so annoyed by the lack of basic features in tape, such as: proper describe/before/after, running tests with grep, missing assertion methods such as match. Also I found out that most of the available reporters are not that great, and the default one is definitely not looking good IMO.

Oh, and one other question, do you have a target Node version you're working towards? Everything I work now is v4+ which is quite nice, I've gotten used to string templates everywhere :)

Node should be 0.12+ I think, because I'm relying on features introduced in that version, such as the pooling implementation in Agent. But I'm fine with supporting 4+, my point in the Styles chapter was about using ES20xx features only when needed, it's not that I'm targeting anything 4- necessarily.

oh, and a quick note on code style, it sounds like you are basically using standard. perhaps just move to standard entirely so that we can use the formatter?

Will take a look at it.

[jabrena]

Hi @mikael in your opinion, what is trend to leave testing with Mocha?

[noway]

Wow, the logs are fabulous. :+1:

Mocha was a nice test framework 2 years ago, shouldn't have had regression on that part since then. A good choice.

[simov]

@froatsnook @noway421 I share your excitement!

@ahmadnassri probably the Har implementation should be moved out of @request/legacy because someone may want to create its own HTTP client based on core, that have a Har support, minus all of the bloat in @request/legacy. In that case I should create a new repo, let me know what do you think.

re testing: Mocha+Should is just a tool, and a good one IMO. I knew that there is a reason behind choosing tape, but over the time using both of them I couldn't figure out why tape is considered better that mocha/should.

As for actually refactoring the entire test suite, that would require moving all of the relevant tests to @request/core. Things to avoid are: huge test servers, creating test cases in a loop, dependence between separate unit tests, and generally things that are hard to debug, maintain, and reason about after their initial implementation.

After that when a change is made in this module's tests, that change should be synced (implemented) in core as well.

[mikeal]

Node should be 0.12+

If you aren't going to support v0.10 then you should just skip straight to v4 as the support story for 0.12 is not great at all.

[mikeal]

My point was that I felt ever so annoyed by the lack of basic features in tape

You may want to check out node-tap. It has become something of a passion project for @isaacs when taking a break from CEOing and it has grown pretty much all the features ever ;)

Not sure what the browserify story is though.

[mikeal]

@jabrena I don't know why people are leaving I've just observed that they are. I was never a big fan to begin with so I'm not entirely sure why but a lot of stuff has moved to node-tap or lab in the last year.

[simov]

@mikeal Support for Node 4+ should be stated explicitly, I agree.

@mikeal both node-tap and lab are looking good, but still I feel like mocha/should is much more expressive.

[nylen]

Great to see this stuff I sort of discussed in passing becoming a reality :)

+1 to switching from tap to mocha.

[eiriksm]

wow nice! Great job!

Personally also a fan of mocha/should, for the record :)

[jabrena]

Nice development!!!

@mikeal, lab and node-tap are interesting but I think that mocha/should have a huge community. Anyway, I will test that libraries. Many thanks for the advice. :)

[netpoetica]

@simov This is epic! One other big benefit of this is that now I can watch the multipart repo, and not have to watch the entire request project - this will help it to be much more maintainable.

I think either @mikeal might be living on the complete edge of node dev or I might be living in a cave because I haven't seen Mocha disappearing into obscurity - it's still very widely used and I definitely support it's usage here

Also, I like the idea of calling this "Request Next" because it reminds me of 5th edition D&D which is called "D&D Next"

Cheers!

Update: Eric Elliot wrote a good article that helps justify the use of tape over Mocha. I agree with all points TBH but I think Mocha doesn't feel like a lot of overhead if you already know and use it. Just thought I would share this article since it is very well done

[simov]

@nylen @eiriksm @jabrena @netpoetica thanks!

I didn't expect the main topic to be the testing framework. I guess it's fairly important for everyone.

Also mikeal made a poll on Twitter.

[jabrena]

Hi @simov, you have reason. :)

[mikeal]

I think either @mikeal might be living on the complete edge of node dev or I might be living in a cave because I haven't seen Mocha disappearing into obscurity - it's still very widely used and I definitely support it's usage here

I ran a quick twitter poll and you are correct, it is still the most used framework even for new projects :)

[mikeal]

One other thing I just thought of, Request Next should deprecate pool and all agent related features.

The agent pool in Node.js v4+ is much better and we should get out of the way. If someone wants to set a different agent we should let them pass it with the agent option but that is it.

For the proxy tunneling we should just handle this tranparently, and probably re-write it since most of its implementation is dealing with legacy agent api.

[ahmadnassri]

@ahmadnassri probably the Har implementation should be moved out of @request/legacy because someone may want to create its own HTTP client based on core, that have a Har support, minus all of the bloat in @request/legacy. In that case I should create a new repo, let me know what do you think.

@simov fully agreed, I've looked through the new code and @request/legacy and trying to figure out the best way to set this up, happy to follow your lead if you've already got a setup in mind, or i can take a stab and share for review.

[simov]

@mikeal yes pool and forever are not supported, you can scan the suite object here, basically excluded: true means the entire test file is excluded, excluded: N means only N tests are excluded. There is a short description down below for each test that is not covered.

Proxy tunneling is supported and all tests are passing, so it behaves much like in request.

Here is the tunneling configuration code, I haven't changed it that much, and it relies on the tunnel-agent which I'm taking for granted :)

[simov]

@ahmadnassri your implementation is almost unchanged, I just removed the switch statements (because styles) and also there was an issue with detecting some of the content-types (not strictly matching), but that was fixed here as well.

The har module itself is called inside the options module right before setting up any other option. This module is a proxy for the options that request accepts, and the defaults it expects, to options that the core understands.

One of my main goals was to keep the core as small as possible and the har option felt like it can live outside of it without a problem (but that may change of course).

So if you take a look at the @request/client for example, an HTTP client that supports the har option might look like this:

var request = require('@request/core') // or @request/client or any other compatible client
var har = require('@request/har')

function client (options) {
  if (options.har) {
    har(options)
  }
  return request(options)
}

module.exports = client

That way you can have a client that supports har without the need to use the legacy module, which is kind of a mess of course :)

So it seems that I just have to create a separate har repo and publish it on npm as well. Let me know if the above scenario suits you.

[simov]

The Sugar API I was talking about.

Implement sugar API - this API can be extremely simple to implement, it just needs to configure the options object before passing it to core.

[stevenvachon]

Feature requests:

• option to retry failed requests a set number of times
• option to retry with get (and abort() before body streams) when head fails with a 405

[stevenvachon]

Progress?

[simov]

Not really. Still it will be much easier for you to pick code from this rewrite to build your own thing.

There is one major issue with this rewrite and that is browserify. If a module is supposed to be optional it shouldn't be required inside that module, but instead passed as interface or configured in some other way.

Also I'm thinking about something even more modular, so that you can reuse certain request related modules with another HTTP client. That means moving even more stuff out of core. For example you may want to use the request interface in the browser along with some of its orbiting modules that essentially only modifies state, but on top of the native fetch API, or jquery.ajax for example :) I think there is some sort of a streaming API in the browser as well.

This will reduce the size of your bundle dramatically and you still will be able to use the same interface both on the server and in the browser.

[stevenvachon]

What is browserify needed for? Why not use it as a devDependency for building a browser distribution file?

[simov]

Still the bundle will be bigger. Way smaller with core than legacy request, but still big. Browserify is not used in request I was talking about code reuse without sacrificing the size of the bundle.

[stevenvachon]

Breaking things apart for reuse is a great idea, but that shouldn't affect this issue. You could use is-browser for deciding which modules to require().

[simov]

Basically what I am proposing is this:

var request = require('@request/core')({
  oauth: require('@request/oauth'),
  multipart: require('@request/multipart'),
  ...
})

That way all dependencies that you need will be bundled with your application, not with the module. The module itself will depend only on certain interfaces, no matter where those interfaces are coming from. Each HTTP client wrapper should obviously implement the same Constructor interface, so that all dependencies stays always in the client application.

Now obviously that's a pseudo code, but it's the easiest way to put it, and it can be extended in future, in case we need a fancier way to include those modules.

So that's a slight deviation from the legacy request module where you just include it and it's ready to be used. The above will be a Closure like Constructor.

With this little change Browserify will actually be able to build the module.

[simov]

Then we can have a convenience module containing a list of all possible request modules:

npm install --save @request/all

And be happy with a 1.8 MB of JS in the browser:

var request = require('@request/core')(require('@request/all'))

Obviously that should be used only for testing, that's the point after all.

[seanstrom]

Very cool refactor :tada:

[simov]

I just wrote a quick specification about the interface that I use in core and what I envision about the optional dependencies. It's not finished yet, but you can quickly read it up to the Interface section, it's very easy and straightforward to understand.

Now why do we even need this?

First of all with this approach anyone would be able to pick and choose what to bundle for the browser.

Second of all having a common interface allows us to build higher level abstractions on top of the HTTP clients. For example I have a module that is a REST API client library. That module accepts HTTP client conforming to the above specification, so I'm able to reuse my higher level module with any underlying HTTP client. I can also fine tune my browser build for various use cases. For example: why should I bundle 400KB of crypto code if I'm not going to use OAuth 1.0 in the browser? Why should I even bundle Node's HTTP implementation if I can use fetch? And so on.

[analog-nico]

I like your spec! Could maybe evolve into something similar like the Promise standard. I already have multiple request implementations in my app because of the combination of libraries I use. With a standardised interface libraries could allow injecting the preferred request implementation.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[ghost]

@simov what is blocking this? Awesome refactor btw!