-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
UI: Add setting to enable/disable blocking queries #5352
Conversation
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
@@ -63,6 +63,9 @@ export default Service.extend({ | |||
}); | |||
} | |||
}, | |||
abort: function(id = null) { | |||
get(this, 'connections').purge(); | |||
}, |
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.
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); |
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.
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> |
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.
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.
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.
Reviewed in person. I agree that the EventSource/caching stuff is getting hairy, but no need to block merging this on that.
This PR:
...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 thecursor
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 allEventSource
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-dataRecord
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 theform
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 terminalmake start
. You can control the latency/response rate of the blocking queries by adding aCONSUL_LATENCY
cookie using web inspector. This value is in milliseconds.