Adds restart method and auxiliar callback to polling class
What does this MR do?
According to the discussion in this MR: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10210
- Adds restart method so we can start polling again after visibility changes back.
- Adds notifiction callback so we can know outside the polling class that we are making a request. This will allow to prevent scenarios where we would make 2 requests at the same time.
- Parses received header into a number.
These changes were moved into this MR so @selfup can use them at the same time and not be blocked
Does this MR meet the acceptance criteria?
- Tests
-
Added for this feature/bug -
All builds are passing
-
-
Conform by the merge request performance guides -
Conform by the style guides -
Branch has no merge conflicts with master
(if it does - rebase it please)
What are the relevant issue numbers?
Merge request reports
Activity
- Resolved by Filipa Lacerda
mentioned in merge request !10210 (merged)
- Resolved by Filipa Lacerda
What is the auxCallback for? Maybe
visualCallback
orupdateStatusCallback
? Up to youWhat status will this update? Will this make a visual change?
Can this be optional? Otherwise we have to pass
() => {}
(I don't mind doing this)Just wondering!
Edited by Regis Boudinot@selfup it should be optional. Will update that! It's not a visual change it simply allows us to know the request is being made, see this https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10210 for details.
@jschatz1 can you please review this one? Thank you!
assigned to @jschatz1
Is there any way you can sneak in:
const pollInterval = parseInt(headers[this.intervalHeader], 10);
as @adamniedzielski did here: https://gitlab.com/gitlab-org/gitlab-ce/commit/74824f1ec9dff1da60812473726076005d67acf5
We won't receive a number @selfup? /cc @adamniedzielski @ayufan I'm expecting a number
@filipa We will definitely set the header value to an integer on the server side. However, I don't know what type you will receive in JavaScript. It's surprisingly hard to find any piece of documentation that describes it. I just noticed https://gitlab.com/gitlab-org/gitlab-ce/blob/ad831ace7ed8d2ed999b15f8350aaa51f0490124/app/assets/javascripts/lib/utils/common_utils.js#L240 which looks like an indication that the header value will be a string not an integer.
Assuming that we use XHR-based implementation for Vue Resource then the relevant piece of code is here and it returns values as strings as per this documentation.
Thank you for explaining @adamniedzielski
will update this asap!assigned to @filipa
@jschatz1 can you please review? :)
assigned to @jschatz1
42 58 43 59 checkConditions(response) { 44 60 const headers = gl.utils.normalizeHeaders(response.headers); 45 const pollInterval = headers[this.intervalHeader]; 61 const pollInterval = parseInt(headers[this.intervalHeader], 10); Why
radix
10
? Isn't that redundant? Or does it come in base 8 or hex or something?Edited by Jacob Schatzradix is not default ("usually defaulting the value to 10") https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/parseInt
MDN says:
Always specify this parameter to eliminate reader confusion and to guarantee predictable behavior.
Edited by Filipa Lacerdaehe always used parseInt with radix, I had some troubles before. We also point to this in our documentation: https://docs.gitlab.com/ce/development/fe_guide/style_guide_js.html#parse-strings-into-numbers
@filipa aside from the radix thing it looks fine. I will approve and merge.
mentioned in commit 15ca592f