timemachine
/
blog
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Proofreading and editing.

This commit is contained in:
Pilot 2020-10-25 12:39:44 -04:00
parent fa088f2344
commit f659d4f246
1 changed files with 49 additions and 52 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

101
source/_posts/code-review-axios.md
View File

@ -8,40 +8,40 @@ tags:
---
> This article is in a series of code review articles that take a
> deep look at a popular module and discus it's merits, flaws, and
> deep look at a popular module and discuss its merits, flaws, and
> overall fitness for a task.
## Summary
[Axios](https://github.com/axios/axios) is a solid, battle tested, replacement for the deprecated `require.js`. I
[Axios](https://github.com/axios/axios) is a solid, battle-tested, replacement for the deprecated `require.js`. I
recommend it despite the imperfections that I have summarized in this article.
The logic in this package is well thought out and meets my standards. Some of
The logic in this package is well thought out and meets my standards. Some of
the more complex functions are arduous to read. This makes collaboration from
the development community difficult and undermines the overall effectiveness of
the project.
The interceptor system is a workable solution for extending the package
functions. Personally, I have wrapped request/response to extend error reporting
functions. Personally, I have wrapped request/response to extend error reporting
and the updates felt natural and a seamless transition.
There are two test runners in the project: Mocha (for node.js) and Jasmine/Karma
(for browser testing). This is unnecessary as both test packages can run both
platforms. A large portion of the tests are written for jasmine and will not
run, without modification in the mocha test suite. This prevents me from
showing full code coverage with out hacking on the tests (more on this later).
platforms. A large number of the tests are written for jasmine and will not
run, without modification in the mocha test suite. This prevents me from
showing full code coverage without hacking on the tests (more on this later).
Running `npm test` takes many minutes and fails, by default, if the developer
does not have the Opera browser installed. I can understand that an exhaustive
integration run in a multi-target package is a long process. Fleshing out the
Mocha test suite to run on the command line in a second npm script would
encourage test driven refactoring and make many of the improvements I outline
much simpler and safer. Iterative, refactoring tests must be fast, sane, and
meaningful. They need not be exhaustive. The exhaustive testing can be saved
encourage test-driven refactoring and make many of the improvements I outline
much simpler and safer. Iterative, refactoring tests must be fast, sane, and
meaningful. They need not be exhaustive. The exhaustive testing can be saved
for pre-release and proofing pull-requests.
The current version is `0.20.0`. There is no explicit roadmap for the project;
however, I do not see a reason for delay in assigning version `1.0.0` to this
however, I do not see a reason for the delay in assigning version `1.0.0` to this
release.
@ -62,7 +62,7 @@ release.
Axios is a promise-based HTTP client. It is available for use in the
browser (wrapping around XMLHttpRequest) or in node.js (wrapping the
built in `http` module.
built-in `http` module.
## Setup
@ -103,7 +103,7 @@ found 33 vulnerabilities (22 low, 10 high, 1 critical)
Installed 975 packages. All but one are dev.
`axios` installs `bundlesize@^0.17.0`, which is a drop in
`axios` installs `bundlesize@^0.17.0`, which is a drop-in
replacement for `du -sh` (if `du` required oAuth read/write access to your
github account). Bundlesize uses a hand full of compression modules, including
`iltorb`. `iltorb` is deprecated garbage.
@ -135,9 +135,9 @@ axios@0.20.0 /home/pilot/Projects/codereview/axios
That is an extremely old version of `ws`. [It has been
fixed](https://www.npmjs.com/advisories/550)
I love `lodash` for the creativity of it's code base, the way the developers
I love `lodash` for the creativity of its codebase, the way the developers
step up to the challenge of being faster than native, but never user it. NPM
awards `lodash`'s prototype pollution (actually polyfils) a `High` level alert.
awards `lodash`'s prototype pollution (actually polyfills) a `High` level alert.
`karma` also depends on an old version of `chokidar`, but [new chokidar is way
cooler](https://paulmillr.com/posts/chokidar-3-save-32tb-of-traffic/).
@ -233,7 +233,7 @@ run passes[^2].
## Code Coverage
This was not as straight forward as I thought it would be. The `package.json`
This was not as straightforward as I thought it would be. The `package.json`
has an entry for `coveralls` but the `lcov` file wasn't generated by the test
run. Looking in `node_modules` I don't see an entry for `blanket.js`. *Another
bad smell.* Checking the `travis-ci` runs for the project they all error on `npm
@ -294,14 +294,14 @@ All files | 79.21 | 62.78 | 79.44 | 79.73 |
-------------------------|---------|----------|---------|---------|-----------------------------------
```
Not bad for a zero config nyc run.
Not bad for a zero-config nyc run.
[See the full report here](/~timemachine/codereview/axios@0.20.0/)
## Digging In
Now that all that boilerplate is out of the way let's look though the code.
Now that all that boilerplate is out of the way let's look through the code.
File by file:
### [lib/axios.js](/~timemachine/codereview/axios@0.20.0/lib/axios.js.html)
@ -318,7 +318,7 @@ This is a general module building/exporting file. A default instance is created:
The bizarre part of this is that all of the above give you different results.
The requarian form is constructed with a set of defaults from
`lib/defaults.js`; the classical constructor has no defaults; and
`lib/defaults.js`; the classical constructor has no defaults;
`axios.create` merges the supplied configuration with the defaults.
@ -326,7 +326,7 @@ The requarian form is constructed with a set of defaults from
Simple and sane defaults. However, the
[`getDefaultAdapter()`](/~timemachine/codereview/axios@0.20.0/lib/defaults.js.html#L16)
function affords me the opportunity to point out one of the stinkiest code
function allows me to point out one of the stinkiest code
smells: *All if … else if constructs shall be terminated with an else clause.*
(See MISRA-C:2004, Rule 14.10, no online links, sorry).
@ -337,17 +337,15 @@ default adapter as `undefined` and the application in an unknown state.
#### Dynamic Imports
While picking on `getDefaultAdapter()`, there are two synchronous `require`
calls. First, I have to say that putting require calls deep in function logic is
calls. First, I have to say that putting requires calls deep in function logic is
a red flag; don't do it, ever. Secondly, it's fine to do it here...
Let me explain by assessing the 3 potential code paths, from the bottom up:
1. The default path: As I explained above the default path is to just return
`undefined`. No harm from the synchronous calls.
2. Browsers: In the context of the browser the `require` function is provided by
webpack. It is a synchronous function; however, webpack barfs all the
javscript assets into the memory on page load. Thus, a synchronous require,
in this context doesn't result in a bad turn[^3].
`undefined`. No harm from the synchronous calls.
2. Browsers: In the context of the browser the `require` function is provided by webpack. The function is synchronous; however, webpack barfs all the javascript assets into memory on page load. Thus, require,
in this context doesn't result in a bad turn[^3].
3. Node.js: [node's module
resolution](https://github.com/nodejs/node/blob/master/lib/internal/modules/cjs/loader.js#L717)
uses a caching system that only reads the file once from disk, the first time
@ -412,9 +410,9 @@ called it an *array* it wasn't really an array but more like an object, with
numerical indexes..."
Targeting old browsers is the pits. It forces you to make hack helper functions
like bind. Necessary evil for pre-IE 9 support[^4].
like bind. A necessary evil for pre-IE 9 support[^4].
Fortunately, axios doesn't care about browsers that old. This should go.
Fortunately, Axios doesn't care about browsers that old. This should go.
#### [lib/helpers/buildURL.js](/~timemachine/codereview/axios@0.20.0/lib/helpers/buildURL.js.html)
@ -436,13 +434,13 @@ to misinterpreted readings by other developers.
#### [lib/helpers/normalizeHeaderName.js](/~timemachine/codereview/axios@0.20.0/lib/helpers/normalizeHeaderName.js.html)
This function takes an object of `[key: string]: string` properties and a second
argument of a key. It then changes the spelling of the key in first argument to
argument as a key. It then changes the spelling of the key in the first argument to
match the capitalization of the second argument.
All header keys in HTTP should be treated as lower case. A more sensible act is
All header keys in HTTP should be treated as lower case. A more sensible action is
to take the input headers and convert them all to lowercase. Then use the
lowercase forms in all interactions. Better still would be to create an
enumeration of the headers axios cared about an then only use references to the
lowercase forms in all interactions. Better still: create an
enumeration of the headers Axios cared about and then only use references to the
enumeration.
#### [lib/helpers/spread.js](/~timemachine/codereview/axios@0.20.0/lib/helpers/spread.js.html)
@ -470,15 +468,14 @@ does a great job of defining the contract for all adapters to agree.
#### [lib/adapters/http.js](/~timemachine/codereview/axios@0.20.0/lib/adapters/http.js.html)
I'll start off my critique by pointing out: [The second edition of Martin
I'll start my critique by pointing out: [The second edition of Martin
Fowler's "Refactoring"](https://martinfowler.com/articles/refactoring-2nd-ed.html)
uses JavaScript for its examples.
The cyclomatic complexity of `httpAdapter` is 42, according to JSHint. That's
just too much. There are some easy wins here for refactoring:
- Creating a new function for converting POST data to a Buffer drops the
complexity by six. The resulting function is easier to test.
- Creating a new function for converting POST data to a Buffer drops the complexity by six. The resulting function is easier to test.
- Transforming the `config.auth` object and jamming it into the URL totals
*ten* paths. Not as easy to refactor as there are some overlapping concerns:
- resolve the `username` and `password`
@ -488,7 +485,7 @@ just too much. There are some easy wins here for refactoring:
- continue to use the parsed URL over the next 90 lines
- Proxy configuration is complex; 60 lines and 13 paths.
- Finally, the callback to `transport.request()` should be it's own function,
and possibly in it's own file.
and possibly in its own file.
#### [lib/adapters/xhr.js](https://github.com/axios/axios/blob/6d05b96dcae6c82e28b049fce3d4d44e6d15a9bc/lib/adapters/xhr.js)
@ -530,7 +527,7 @@ is an instance of an error.
A CancelToken is an identifier that triggers the cancellation process.
The class has a factory to create a `source` and a constructor that does not
return a `source`. Instead it returns a function. This is confusing. I
return a `source`. Instead, it returns a function. This is confusing. I
recommend:
- `CancelToken` should return a `source`. This would unify the two styles of
@ -540,7 +537,7 @@ recommend:
is present.
- Optionally: warn about deprecation when `source` is called as a function.
(there would be different styles of warning for Browser vs. Node.js so
feasiblity is debatable)
feasibility is debatable)
- Update the documentation to only use the `source` style cancellation.
### lib/core
@ -555,7 +552,7 @@ configuration for the Requests and Responses.
The first line of the function is an eslint suppression comment. Manipulating
the config of static analysis tools at run time is sometimes needed; however, it
should be the exception and accompanied by a large amount to explanation
should be the exception and accompanied by a large amount of explanation
comments.
It's such a short function I'll just show you how this function should be
@ -573,11 +570,11 @@ module.exports = function(data, headers, transforms) {
In this trivial function, the usefulness of the `no-param-reassign` seems
suspect. Function bodies should treat all parameters as immutable, never assign
new values to them and it is absolutely crucial to never preform property
new values to them and it is absolutely crucial to never perform property
reassignment to parameters. Function purity is one of the best defenses we have
against defects.
Like I said, this small function is easy to understand and there is little
As I said, this small function is easy to understand and there is little
chance of a bug finding its way in here, but practicing proper habits when the
complexity is low sets us up for success when tackling the 42 headed hydra of
`lib/adapters/http.js`.
@ -592,7 +589,7 @@ I'll explain the next few files as a group.
The two adapters (xml and http) both pass their responses along with the promise
callbacks to `settle`. Passing resolve/reject to a function is a bit thick.
Settle can just return a promise, if it needs asynchrony (it doesn't).
Settle can just return a promise if it needs asynchrony (it doesn't).
Settle looks for the existence of the dubiously named: `validateStatus`
function and calls it to determine if the response was a success or error.
@ -614,7 +611,7 @@ Error object and then passes the error along with it's the other arguments to
`enhanceError` adds the request config, response code, complete request, complete
response, and a new property (`isAxiosError`) to the error object before doing
something that is seemingly bizarre: it arguments the error with a new function
called, `toJSON()` that just makes a copy of it's self...But why[^6]?
called, `toJSON()` that just makes a copy of its self...But why[^6]?
Take this example from the node REPL:
@ -626,7 +623,7 @@ Take this example from the node REPL:
Yep. You can't encode an `Error` object as JSON...*sadface*...Luckily, JSON
provides a canonical way of tackling the problems of JavaScript. The `stringify`
function inspects the objects (recursively) for a `toJSON` method. If found the
result of calling `toJSON` is encoded in lieu of the parent object:
result of calling `toJSON` is encoded in place of the parent object:
```
> const e = new Error('bad thing');
@ -688,7 +685,7 @@ compatibility to IE 9+.
(Editor's Note: the README only claims compatibility for IE 11+)
Cloning objects is needlessly hard in JavaScript (how many versions before
`Object.prototype.clone()`? ...its possible). If you have need of such arcane
`Object.prototype.clone()`? ... it's possible). If you need such arcane
transformations study the [MDN page on the subject](https://wiki.developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign).
Honestly, if you can't `Object.assign(a, JSON.parse(JSON.stringify(b)));` what's
@ -723,7 +720,7 @@ sets up the interceptors.
You can see the *Chain of Responsibility Pattern*, as a literal `chain`
variable. The chain is initialized with the dispatcher and undefined (you need
an even number of links in the chain, because: *shenanigans*). All of the
pre-request interceptors are un-shifted to start of the chain (even numbers
pre-request interceptors are un-shifted to the start of the chain (even numbers
again for a fulfilled or rejected promise). Likewise of the post-request
interceptors are pushed to the end of the chain. Finally, two of the
interceptor-callbacks are removed from the head of the chain and added as
@ -731,18 +728,18 @@ interceptor-callbacks are removed from the head of the chain and added as
the case of the dispatcher it handles its own rejection so we need an extra
element in the chain *shenanigans!*.
The master promise resoles all the pre-request hooks, the dispatcher, and all
the post-request hooks in order. **Study this code till you understand how it
The master promise resolves all the pre-request hooks, the dispatcher, and all
the post-request hooks in order. **Study this code until you understand how it
works!**
## In Closing
I use the axios library, professionally, to send business critical requests to
I use the Axios library, professionally, to send business-critical requests to
third-party servers. When software is critical to your business you must be
critical of the software. However, the majority of software available through
the Node Package Manager passes without scrutiny.
From the length of this article you can guess that this was a multi-week effort
From the length of this article, you can guess that this was a multi-week effort
to type up all my thoughts. I did as much research on my claims as I thought was
reasonable to give an accurate representation. The first draft was not perfect
and I have made revisions as my understanding of the source code evolved.
@ -751,10 +748,10 @@ While passionate about code quality I also have a sense of humor; my hope is
that both aspects enrich the reading experience and that my wit does not
distract the reader.
Thank you to my proof readers and technical editors, without them this would be
Thank you to my proofreaders and technical editors, without them this would be
awful.
I have included additional action items in some the footnotes and will update
I have included additional action items in some of the footnotes and will update
them with links to any pull requests that result so that the reader can stay
abreast of my contributions.