refactor: remove unused batching code and remove duplication #343
Conversation
pablopalacios
force-pushed
the
remove-batching
branch
from
ac66437
to
a10b02c
Compare
libs/fetcher.js
Outdated
| return resource | ||
| ? resource.replace(RESOURCE_SANTIZER_REGEXP, '*') | ||
| : resource; | ||
| return resource ? resource.replace(/[^\w.]+/g, '*') : resource; |
There was a problem hiding this comment.
Was this to avoid a bug? We usually keep the regex out to avoid recreating the regex object each request
There was a problem hiding this comment.
Oh, I didn't know that. I'll remove this commit.
|
I went commit by commit and it looks pretty good to me, you can rebase since the other PR was merged. Then I can leave any feedback on removed badge code changes. |
pablopalacios
force-pushed
the
remove-batching
branch
from
a10b02c
to
24aa6d3
Compare
pablopalacios
force-pushed
the
remove-batching
branch
from
24aa6d3
to
48c516d
Compare
| @@ -10,65 +10,14 @@ | |||
| * @module Fetcher | |||
| */ | |||
| var REST = require('./util/http.client'); | |||
| var DEFAULT_GUID = 'g0'; | |||
| var defaultConstructGetUri = require('./util/defaultConstructGetUri'); | |||
There was a problem hiding this comment.
looks like this file can move to modern code too, for another PR
There was a problem hiding this comment.
Oh, only if we completely drop support for IE11 (or add ts or babel to transpile correctly).
There was a problem hiding this comment.
Yup, I created #344 to convert to TS
| @@ -26,6 +26,17 @@ var DEFAULT_CONFIG = { | |||
|
|
|||
| var INITIAL_ATTEMPT = 0; | |||
|
|
|||
| function parseResponse(response) { | |||
There was a problem hiding this comment.
this was in fetcher.client.js but not needed there?
There was a problem hiding this comment.
It is not that it's not needed. But it's inconsistent that we parse error responses inside the http module but normal responses we parse inside the fetcher module. Now it's everything in one place (and we can potentially use fetch().(response => response.json())) instead of text() (see:
fetchr/libs/util/http.client.js
Line 223 in 80923dc
| @@ -36,12 +35,55 @@ function parseParamValues(params) { | |||
| }, {}); | |||
| } | |||
|
|
|||
| function parseRequest(req) { | |||
There was a problem hiding this comment.
some of these functions could probably move to their own file as well to reduce the fetcher.js size
There was a problem hiding this comment.
Perhaps with the TS migration we can re-organize the folders. It's clear to me that we have actually to packages here, one for the server, another for the client. It seems wrong to have everything in the same folder as we have right now. So I'm avoiding to split the server code until we don't have things better organized (by the way, utils has only client side code now).
|
Released in 0.7.2 |
Support for batched requests was drop a long time ago (see: #110) but we still have some code from that time.
By removing it completely, GET and POST requests are almost identical (the only difference is that POST has a body). This allows us to do some refactoring to remove many code duplication.
The change is quite big (and is on top of #342, so I recommend to merge that one first) but hopefully, reviewing commit by commit should make it easier to review.
I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.