Zero abstraction that leads to minimalistic code base. Which is fine, but refactoring is needed on each stage of the software development, otherwise it turns into a giant mess of entangled quick fixes, instead of having real features.
Adding features requires more time, but even more importantly, it requires someone (or a group of people) to watch for and enforce the design goals. Without such people and a clear design on each stage of the development, that software project inevitably slows its growth and ultimately dies. Well, in the case of request it's still going to be used for a long time ahead, but no one wants to support it anymore.
Another negative impact of not having leadership is that the project's code base grows unproportionally in size and complexity, without its design evolving into a smaller and simpler components. The result is a huge chunk of code that no one have the time, energy and dedication to rewrite.
When I started working on request, about a year ago, most of its code base was in a single file with around 2k lines of code. Logically the first step was to extract the most obvious features into separate modules. The goal was to reduce the amount of code in request.js. As a rule of thumb refactoing always starts from the most obvious parts in the code base. The refactoring is a short sighted activity where you concentrate on a single task only. The goal is to improve the code just a little bit, not to fix at a whole. Once you achieve your current goal, without introducing complexity in another place, you can move on to the next step. It's also important to note that failing while refactoring is totally fine. If you try a few times and it still doesn't feel good, just revert back your changes, take a step back, and think about the new challenges ahead, and possibly move to another area of the code base.
At first glance what I did there in the lib folder looks trivial - just copy/paste some code in new files. But the fact is that it required a lot of consideration about retaining the complexity level and not breaking existing clients. That sort of worked for some of the modules, until I decided to extract the redirect logic. The idea behind creating separate modules is to reduce the amount of state attached to the global request instance and move those state variables related to a feature into their respective module. It's a fairly trivial task of reducing scope and capsulating functionality.
All new module instances are attached to the request instance as private properties via underscore prefix. The problem is that no one have ever though about specifying which state variables should be considered public, and which private. That small decision made the refactoring impossible. There is no way for me to know which state variable each one of the 9k depending on request modules uses. Even if the API was defined as private I would still have major headaches, but if every single state variable is public, then my hands I tied.
Anyway, I wanted to change so much more than just the state, still my efforts there proved to be useful, and these efforts were the building blocks for the following rewrite. A good example about the benefits of moving stuff into modules was the introduction of the Har module. This was the first module implemented externally from the start, and it follows the example set by the rest of the modules in the lib folder.
The next logical step was to start looking for a way to rewrite the module. Mikeal had such rewrite started in the past, using newly built Streams2 compatible HTTP duplex client, but what I saw there was basically the foundations of the exact same architecture with the only difference being that it uses Streams2.
So I started working on my own HTTP client based on the Mikeal's HTTP duplex client. I started with creating a high level proof of concept tests. These tests are like code examples showing how I want to interact with the module.
A few rules:
- The core is completely transparent, it supports all of the HTTP core request options directly and it acts like it by default. The only difference is that the returned object is a duplex stream plus a bit of logic added here and there around the public stream API methods.
- The core supports additional features as well, but each feature is being enabled explicitly via option property. Meaning that there are no defaults.
- The core implements only HTTP features.
The Core's sole purpose is to ensure that all HTTP features in it can work together. The core is not the place for opinions.
If I have to describe the core with 3 words they would be: dumb, consistent and predictable.
A couple of rules:
- The request instance (the HTTP duplex client) doesn't hold any state other than a couple of private state flags.
- The options object that the user passes holds the rest of the state. The idea is that this is the only thing passed to the initial request and any other subsequent redirect.
- The initialization of the initial request, and the initialization before each redirect is separated. That way it's easier to know which features have logic executed on response.
- Each feature/option module can export only two methods - request and response. The request method defines logic to be executed on the initial request. The response method defines logic to be executed on response.
- A feature/option can be accessed only by the core itself. Features cannot interact with each other directly, that is to ensure they do not depend on each other and that they can be used separately.
- Features that have external dependencies are marked as such in the documentation. If some of their external dependencies is not installed the core throws a runtime error.
All supported HTTP core request options and the ones specific to this @request/core module are defined explicitly. Any other option passed to core is being filtered out on initialization. The core generates internal representation of the user's options and filters them once again before passing them to the underlying HTTP core request method.
All of the orbiting modules published in the @request namespace should be neutral to core, meaning they can be used without it. Good examples for such modules are the @request/oauth and the @request/multipart modules.
Speaking of multipart - that's one of the reasons why the original request module isn't migrated to Streams2 yet. The form-data module is depending on combined-stream, which in turn is strictly Streams1 one.
The new multipart module can be used to generate any multipart body including form-data and related. I ended up writing my own combined-stream like replacement using Streams2 called bulk-stream. I'm not a streams expert and I still feel funny each time I have to deal with streaming code. If there is a better alternative to bulk-stream I would gladly use that instead.
At that stage I had the foundations laid out. But I had only my too generic proof of concept tests. I started working on an opinionated HTTP client that consumes the core. I needed such client in order to test the core against the request's test suite.
In this new architecture the user is encouraged to create its own opinionated HTTP client. Generally such client can define its own set of default options along with their dependencies. Sugar API and any other non HTTP related features can be implemented in this layer as well.
To sum it up, an opinionated HTTP client can:
- Set default options to use.
- Take care of specifying the needed dependencies.
- Implement sugar API - this API can be extremely simple to implement, it just needs to configure the options object before passing it to core.
- Implement complementary features, not directly related to the inner workings of the core.
The most basic example of such HTTP client is the @request/client. The only thing it sets is the end option to be used by default. That feature/option tries to end the request in process.nextTick without the need of calling the request's end method explicitly.
The @request/legacy client on the other hand contains much more than that. Some of the original request features are implemented entirely in this layer. Some of them can be further extracted into separate modules to ease the authors of custom HTTP clients. Part of the code there is just a proxy for the original request options and their counterparts in @request/core. Additional methods, copy state just to make the tests pass, and so on.
The current @request/legacy module is not 100% compatible with the original request module, though it's pretty close and it covers nearly the entire test suite.
Complete list of all tests that are covered by the @request/legacy and the @request/core duo, plus the ones that are not can be found here. Generally if a test is not covered there is a good reason for that, and it's thoroughly documented.
Currently my flow is as follows:
- I have a local clone of the request repository on my workstation that have everything ignored and removed except the tests folder.
- Each time there is a change in the original request tests, I pull them up in this local repository.
- Then I pull them once again in the @request/legacy repository to sync the test suite. The tests in @request/legacy are never modified directly.
- Lastly I'm applying the needed changes to @request/legacy and/or @request/core.
A couple of things to note about the current test suite:
The test suite in request follows the DRY pattern, logically, but in the context of unit testing the DRY pattern sometimes contradicts with the rule for atomicity of the unit test. So instead of following design patterns blindly, it's always a good idea to think through the requirements of the task at hand. Some test files are so complex that they are like applications on their own. It's not so uncommon to spend 20 minutes implementing a feature and then 1 hour refactoring a test file just to make sure the person after you won't start looking for your address immediately. And the chances are that this person might as well be you.
The point being is that in some cases the test server is too complex, it's on top of a 600 lines of code, or in another file. In some cases the tests depend on each other, so you can't isolate and run a unit test separately. It's like no one have ever thought about that you may want to concentrate and work on a specific feature only, instead of running all of the 50 other tests in that file and find your way through the output logs each time.
Things I would like to see:
I'm personally using mocha as a test framework because it helps you with defining the test cases properly. In combination with should it's just better than tape in any case. Both modules are supported very well to these days and mocha have a tap reporter if you insist on that.
Basic unit test might look like this:
var http = require('http')
var request = require('@request/core')
describe('manual read and manual write', function () {
var server
before(function (done) {
server = http.createServer()
server.on('request', function (req, res) {
req.pipe(res)
})
server.listen(6767, done)
})
it('yep that works too', function (done) {
var input = fs.createReadStream('cat.png', {highWaterMark: 1024})
, output = fs.createWriteStream('output.png')
var req = request({method: 'POST', url: 'http://localhost:6767'})
input.on('readable', function () {
var chunk = input.read()
if (chunk) req.write(chunk)
else req.end()
})
req.on('readable', function () {
var chunk = req.read()
if (chunk) output.write(chunk)
else output.end()
})
output.on('close', function () {
var stats = fs.statSync('output.png')
stats.size.should.equal(22025)
done()
})
})
after(function (done) {
server.close(done)
})
})When needed each test should have its own basic server set up with the cases needed for that test only. If you are worried about code repetition, then you can always use multiple cursors or replace in file. If you follow a consistent format for all test cases that won't be a problem, and to be honest these types of changes are needed very rarely.
Executed with grep each test case can be debugged separately:
$ mocha --debug-brk -g 'manual read and manual write' test/duplex-stream.jsProblem solved.
Unfortunately at this point I don't think I'm going to invest that much time and effort in re-writing the entire test suite. Especially if this new architecture is not going to be adopted in some shape or form.
$ DEBUG=req,res,body mocha -g 0 ~/request/core/test/auth.js
Similar to request-debug for request, the core have a logger module. But this @request/log module is pretty simple, it just subscribes for a bunch of events that the core emmits.
The logger must be installed separately before it can be used and it should be enabled via DEBUG environment flag. You can read more about all available options that you can pass to the DEBUG variable here. There is a room for improvements in this module, but even now the output is fancy enough for most of the use cases, and it proved to be a valuable tool while debugging.
Although very rudimentary at this point, you can use the @request/cli module as a guidance for creating your own command line tool (not mentioning curl here).
$ DEBUG=req,res,json request -u https://api.npmjs.org/downloads/point/last-day/request -p "{json: true}"
Pick a meme of your choice.
If there are not enough people upset already, let's talk about code conventions!
A few rules:
- 2 space indentation
- no semicolons
- space between function and ()
- space between if and ()
- else and else if are always on new line
- no switch statement
- triple equality operator - just to make you feel safe and happy
.. dumb, consistent and predictable.
Probably the most annoying item from this list is the space between function and (). I personally use snippets for these little things, so my code looks consistent without me thinking about it all the time. I can also refactor much faster that way.
<snippet>
<tabTrigger>f</tabTrigger>
<content><![CDATA[
function ($1) {
$0
}
]]></content>
<scope>source.js</scope>
</snippet>Yep, that's exactly how much time I spend writing that function statement.
I just love people hating me for no good reason.
Currently I'm not using any of the latest ES syntax sugar features in any of the modules in the @request namespace. I like ES 20xx features a lot and I think each one of them have its own good use. But just like following design patterns blindly, syntax sugar alone won't make your maintainability and design decisions magically disappear.
If we are not gaining anything meaningful from that particular language construct or a keyword, then we don't use it for that particular situation. We should be focusing on design, not focusing on "which keyword should I put on that particular line". The language itself should be almost transparent, and we should be thinking about what we want to implement, instead of how to implement it (in terms of language constructs). And to be honest, the current code base for all of the modules in the @request namespace is so simple and straightforward, that I can't possibly justify using any of the new language constructs and keywords.
Arguments like: "use let because that's what most people expect from scoping" and using const instead of var all over the place "because that signify your intentions more clearly", in the context of relatively small utility module, are just silly. Not to mention that the first step toward unmaintainable code happens when you start perceiving your users as fools.
Similar to the original request module, all modules in the @request namespace are licensed under the Apache License, Version 2.0.
Each module author's field point to my name. After all I'm the one who spent 3 months of his spare time putting it all together.
There is a notice section at the bottom of each each module's readme file stating that:
This module may contain code snippets initially implemented in request by request contributors.
If there is a need to, I can add a NOTICE file as well.
I'm going to use @request/core in all of my future projects, though probably through the @request/client. I'm still active in the original request issue tracker and I'm taking care of synchronizing the new features and bugfixes in request with core. Currently the core's development is not that pleasant because you have to juggle with a few modules each time: request's tests, @request/core and @request/legacy.
If this code base gets adopted simply @request/legacy can be the next major release for the request module, so that we won't lose traction, GitHub fame, or anything else.
Without greater exposure of this new code base it probably won't get much further, so I and Mikeal decided that it's just fine to push it under the request organization. Of course nowadays there are plenty of HTTP clients to choose from, so this new architecture is just another spin on the matter, hopefully a better one.
Simo