Skip to content
Snippets Groups Projects

Adds restart method and auxiliar callback to polling class

Merged Filipa Lacerda requested to merge 5983-poll-changes into master
1 unresolved thread

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?

What are the relevant issue numbers?

#5983 (closed)

/cc @jschatz1 @selfup

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Filipa Lacerda mentioned in merge request !10210 (merged)

    mentioned in merge request !10210 (merged)

  • Filipa Lacerda resolved all discussions

    resolved all discussions

  • Yea we can call it auxiliaryCallback or auxCallback

    Up to you :tada:

  • What is the auxCallback for? Maybe visualCallback or updateStatusCallback? Up to you :joy:

    What 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
  • Author Contributor

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

  • Filipa Lacerda added 1 commit

    added 1 commit

    Compare with previous version

  • Filipa Lacerda resolved all discussions

    resolved all discussions

  • Author Contributor

    @jschatz1 can you please review this one? Thank you!

  • 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

  • Author Contributor

    We won't receive a number @selfup? /cc @adamniedzielski @ayufan I'm expecting a number :thinking:

  • @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.

  • Author Contributor

    Thank you for explaining @adamniedzielski :muscle: will update this asap!

  • assigned to @filipa

  • Filipa Lacerda added 1 commit

    added 1 commit

    • 81ed06cb - Parses string header into a number

    Compare with previous version

  • Filipa Lacerda marked the task All builds are passing as completed

    marked the task All builds are passing as completed

  • Author Contributor

    @jschatz1 can you please review? :)

  • 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);
  • @filipa aside from the radix thing it looks fine. I will approve and merge.

  • Jacob Schatz approved this merge request

    approved this merge request

  • Jacob Schatz resolved all discussions

    resolved all discussions

  • merged

  • Jacob Schatz mentioned in commit 15ca592f

    mentioned in commit 15ca592f

  • Please register or sign in to reply
    Loading