Skip to content

Keep up/cancel subscription requests fix.#154

Merged
YegorUdovchenko merged 10 commits intomasterfrom
keepup-subscriptions
Oct 22, 2020
Merged

Keep up/cancel subscription requests fix.#154
YegorUdovchenko merged 10 commits intomasterfrom
keepup-subscriptions

Conversation

@YegorUdovchenko
Copy link
Collaborator

@YegorUdovchenko YegorUdovchenko commented Oct 18, 2020

This PR brings changes enabling sending of requests for keeping up and cancelling Firebase subscriptions.

The following fixes were made:

  • remove methods of FirebaseSubscriptionService making it run() and stop(). This service now can run and stop itself depending on presence of subscriptions it manages.
  • fix callback notifying subscribers of entity (event) subscriptions, that a subscription is closed.
  • added integration tests making sure that /subscription/keep-up and /subscription/cancel requests are sent correctly during a subscription lifecycle.

@YegorUdovchenko YegorUdovchenko self-assigned this Oct 18, 2020
@YegorUdovchenko YegorUdovchenko changed the title Keepup subscriptions Keep up/cancel subscription requests fix. Oct 18, 2020
@YegorUdovchenko YegorUdovchenko added the Client JS Issues related to the `spine-web-client` library label Oct 18, 2020
@codecov
Copy link

codecov bot commented Oct 19, 2020

Codecov Report

Merging #154 into master will increase coverage by 0.09%.
The diff coverage is 34.78%.

@@             Coverage Diff              @@
##             master     #154      +/-   ##
============================================
+ Coverage     61.79%   61.88%   +0.09%     
  Complexity      178      178              
============================================
  Files            88       88              
  Lines          2188     2188              
  Branches         40       40              
============================================
+ Hits           1352     1354       +2     
+ Misses          827      825       -2     
  Partials          9        9              

@YegorUdovchenko
Copy link
Collaborator Author

@dmitrykuzmin PTAL

@YegorUdovchenko
Copy link
Collaborator Author

@dmdashenkov PTAL


/** Interval in milliseconds for sending subscription keep up requests. */
_keepUpInterval() {
return SUBSCRIPTION_KEEP_UP_INTERVAL.inMs();
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this configurable. I think, it is configurable on the server side.

});

function subscribeToAllTasks() {
const topic = client.newTopic()
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be shortened as

return client.subscribeTo(Task).post();

}

/** Interval in milliseconds for sending subscription keep up requests. */
_keepUpInterval() {
Copy link
Contributor

@dmitrykuzmin dmitrykuzmin Oct 19, 2020

Choose a reason for hiding this comment

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

If this method stays unchanged after addressing @dmdashenkov request, it should be marked @private like all others.

@YegorUdovchenko
Copy link
Collaborator Author

@dmitrykuzmin @dmdashenkov PTAL again.

@YegorUdovchenko YegorUdovchenko merged commit 9118d1f into master Oct 22, 2020
@YegorUdovchenko YegorUdovchenko deleted the keepup-subscriptions branch October 22, 2020 11:26
@yuri-sergiichuk yuri-sergiichuk mentioned this pull request Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Client JS Issues related to the `spine-web-client` library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants