-
Notifications
You must be signed in to change notification settings - Fork 233
Batch Content Redesigned Implementation #116
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
Conversation
|
@MIchaelMainer Have done the changes that you have requested. Please have a look. |
| * @param {BatchRequestStep} request - The request value | ||
| * @return The id of the added request | ||
| */ | ||
| addRequest(request: BatchRequestStep): string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to return the id of the added Request? Could it be a 'void' method..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of void, returning id gives the sense that the request is been added. This is why we are returning id.
| if (self.requests.has(request.id)) { | ||
| let error = new Error(`Adding request with duplicate id ${request.id}, Make the id of the requests unique`); | ||
| error.name = "Duplicate RequestId Error"; | ||
| throw error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so what happens at the caller if we throw from here..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error is thrown to the caller and the caller of this method have to handle this.
src/BatchRequestContent.ts
Outdated
| requestData.body = await BatchRequestContent.getRequestBody(request); | ||
| } | ||
| /** | ||
| * Check any other property needs to be used from the Request object and add them |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Check any other ...
it helps in textual searching for TODOs later on.
| } | ||
| if (!bodyParsed) { | ||
| try { | ||
| if (typeof Blob !== "undefined") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this Blob ? some global ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, Blob is a global object present only in the browser environment. Since code is being used both by node and browsers, we have this check
| body = await new Promise(resolve => { | ||
| reader.addEventListener("load", function () { | ||
| let dataURL = <string>reader.result, | ||
| regex = new RegExp("^\s*data:(.+?\/.+?(;.+?\=.+?)*)?(;base64)?,(.*)\s*$"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is generally a good idea to add a couple of examples in the comments for regular expression patterns.
0ce7751
deepak2016
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Batching
Batching is a way of combining multiple requests to resources in same/different workloads in a single HTTP request. This can be achieved by making a post call with those requests as a JSON payload to $batch endpoint.
BatchRequestContent
Component which eases the way of creating batch request payload. This class handles all the batch specific payload construction and stuffs, we just need to worry about individual requests.
There are more functionalities like removeRequest, addDependency, removeDependency.
BatchResponseContent
Component to simplify the processing of batch responses by providing functionalities like getResponses, getResponseById, getResponsesIterator