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

UI: Add setting to enable/disable blocking queries #5352

Merged
merged 1 commit into from
Feb 21, 2019

Conversation

johncowen
Copy link
Contributor

@johncowen johncowen commented Feb 15, 2019

This PR:

  1. Re-enables the settings page
  2. Adds a toggle to enable/disable blocking queries at a UI level
  3. Ensures that the setting isn't cached in the event source cache
  4. Ensure that changing the setting cancels any existing requests
  5. Also ties up cancelling and restarting requests on tab switch

...and is to be merged down onto #5070

Notes:

On/off setting cache

Previous to this, the on/off setting was a planned addition to the EventSource/blocking queries/live updates feature. Until now I'd simply used a manual edit of localStorage to test this (see #5267).

Here we've added the on/off toggle and once we did this I noticed that the on/off setting was also being cached in the EventSource caching code. This meant that changing the UI setting after an EventSource had been cached had no effect. An unplanned hiccup! As well as adding the UI toggle here, we've re-plumbed this setting slightly in the caching code to make sure that it works properly there.

The setting is off by default, I would imagine we may release like this and then make it default enabled at a later date, but thats open for discussion.

On a related note, I had previously thought this cache does too much, and the way we've approached this could be simplified further. We essentially cache the EventSource entirely rather than the cursor and the last response/resultset. I had thought to change this but figured that in the scheme of things it's not that important - now I've realized the little hiccup we've fixed here. It gives me more impetus to slightly rearrange this so the cache does less. Potentially we could replace the caching with something form ember core as we should be able to remove all EventSource related things from the caching code. In saying that, it's still low down on my list of things to do,

Settings page

We'd left the settings page in but disabled for a little while, this uncomments the code to enable the settings page again. We removed a lot of the existing code as its no longer needed now the ACL login form is in the actual ACLs area.

Settings form

Ideally we would have like to use my form work here. But as it's not backed by an ember-data Record it's slightly different and will need a tiny bit of code adding to support that. I'd rather do this in a separate PR as it's not really related to this. So for the moment the settings form uses a simpler approach. The form is really simple anyway so its no biggie either way, but once settings gets more complicated we'd rather use the form work.

We've added some todo notes in here for when we come to do this additional work.

Aborting on tab 'hide' (and then restarting)

We also plumbed in the work from #5083 to restart aborted requests here. It's not really related to this specific PR, but felt it best to get this in here so we can get everything merged down ASAP.

Apart from adding more tests, and generally kicking the tyres a lot, the only further thing we may add before merging this in to my base branch will be looking at some sort of automatic rate limiting. We currently use a 'throttle' setting, which I'd imagined would be in the settings page. We're holding off on doing this just now until I have chance to look at something something automatic.

I'll add some inline comments here also.

Just incase you can try this after a clone by starting the fake API using make start-api then in another terminal make start. You can control the latency/response rate of the blocking queries by adding a CONSUL_LATENCY cookie using web inspector. This value is in milliseconds.

screenshot 2019-02-15 at 13 43 48

1. Reenables the settings page
2. Adds a toggle to enable/disable blocking queries at a UI level
3. Ensures that the setting isn't cached in the event source cache
4. Ensure that changing the setting cancels any existing requests
5. Also ties up cancelling and restarting requests on tab switch
@johncowen johncowen added the theme/ui Anything related to the UI label Feb 15, 2019
@johncowen johncowen requested a review from a team February 15, 2019 13:48
@@ -63,6 +63,9 @@ export default Service.extend({
});
}
},
abort: function(id = null) {
get(this, 'connections').purge();
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've included an id argument here, to remind us at some point that it maybe useful to be able to abort individual connections here. We initially named it abortAll, and then changed to just abort when we figured this might be useful.

@@ -38,20 +39,27 @@ const createProxy = function(repo, find, settings, cache, serialize = JSON.strin
const status = get(e, 'errors.firstObject.status');
if (status === '0') {
// Any '0' errors (abort) should possibly try again, depending upon the circumstances
// whenAvailable returns a Promise that resolves when the client is available
// again
return client.whenAvailable(e);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This returns a Promise which resolves when the client (tab) becomes available again. As this is all based on a recurring Promise this essentially pauses the blocking query when the status of the error is zero (aborted) and then resolves the promise, and hence restarts the blocking query.

<div class="type-toggle">
<label>
<input type="checkbox" name="client[blocking]" checked={{if item.client.blocking 'checked' }} onchange={{action 'change'}} />
<span>Enable Catalog realtime updates (blocking queries)</span>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to be specific what this does as I've no idea when this will land in a release. We may be able to add in blocking queries to the rest of the app before then, we may not.

Copy link

@DingoEatingFuzz DingoEatingFuzz left a comment

Choose a reason for hiding this comment

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

Reviewed in person. I agree that the EventSource/caching stuff is getting hairy, but no need to block merging this on that.

@johncowen johncowen merged commit 5534187 into feature/ui-event-source Feb 21, 2019
@johncowen johncowen deleted the feature/ui-event-source-settings branch February 21, 2019 10:15
johncowen added a commit that referenced this pull request Feb 21, 2019
- Maintain http headers as JSON-API meta for all API requests (#4946)
- Add EventSource ready for implementing blocking queries
- EventSource project implementation to enable blocking queries for service and node listings (#5267)
- Add setting to enable/disable blocking queries (#5352)
johncowen added a commit that referenced this pull request Feb 21, 2019
- Maintain http headers as JSON-API meta for all API requests (#4946)
- Add EventSource ready for implementing blocking queries
- EventSource project implementation to enable blocking queries for service and node listings (#5267)
- Add setting to enable/disable blocking queries (#5352)
johncowen added a commit that referenced this pull request Apr 29, 2019
- Maintain http headers as JSON-API meta for all API requests (#4946)
- Add EventSource ready for implementing blocking queries
- EventSource project implementation to enable blocking queries for service and node listings (#5267)
- Add setting to enable/disable blocking queries (#5352)
johncowen added a commit that referenced this pull request May 1, 2019
- Maintain http headers as JSON-API meta for all API requests (#4946)
- Add EventSource ready for implementing blocking queries
- EventSource project implementation to enable blocking queries for service and node listings (#5267)
- Add setting to enable/disable blocking queries (#5352)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants