Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: remove unused batching code and remove duplication #343

Merged
merged 8 commits into from
Dec 28, 2021

Conversation

pablopalacios
Copy link
Contributor

@pablopalacios pablopalacios commented Dec 27, 2021

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.

libs/fetcher.js Outdated
return resource
? resource.replace(RESOURCE_SANTIZER_REGEXP, '*')
: resource;
return resource ? resource.replace(/[^\w.]+/g, '*') : resource;
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this to avoid a bug? We usually keep the regex out to avoid recreating the regex object each request

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I didn't know that. I'll remove this commit.

@redonkulus
Copy link
Contributor

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.

@@ -10,65 +10,14 @@
* @module Fetcher
*/
var REST = require('./util/http.client');
var DEFAULT_GUID = 'g0';
var defaultConstructGetUri = require('./util/defaultConstructGetUri');
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like this file can move to modern code too, for another PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, only if we completely drop support for IE11 (or add ts or babel to transpile correctly).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, I created #344 to convert to TS

@@ -26,6 +26,17 @@ var DEFAULT_CONFIG = {

var INITIAL_ATTEMPT = 0;

function parseResponse(response) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this was in fetcher.client.js but not needed there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

response.text().then(function (responseBody) {
). This change was not really related to this PR but it was lingering in my machine for an while.

@@ -36,12 +35,55 @@ function parseParamValues(params) {
}, {});
}

function parseRequest(req) {
Copy link
Contributor

Choose a reason for hiding this comment

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

some of these functions could probably move to their own file as well to reduce the fetcher.js size

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

@redonkulus redonkulus merged commit 6c51d5a into yahoo:master Dec 28, 2021
@redonkulus
Copy link
Contributor

Released in 0.7.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants