UI: Replace the adapter cancellation methods with a cancellation token system

[DingoEatingFuzz]

This replaces the way that XHR cancellation happens throughout the app.

Before

Cancellation was handled by adapters. Adapters would track all the XHRs and exposed methods that took adapter-like arguments (model name and ID). The cancel method would then look up the XHRs in the registry and abort them.

This had one major issue: there was no discerning between two requests to an identical endpoint.

This may seem like an edge case, but it's oddly easy to end up in this situation. If Page 1 is watching record A and then the app transitioning to Page 2 that tries to load record A in the model hook, the order of events goes:

1. Request to record A is made (watchRecord)
2. Page transition
3. Request to record A is made (model hook)
4. Cancellation for record A is made
5. Both requests are aborted
6. Model hook throws an error due to an empty response (AbortError)

After

XHRTokens are created at the point where watch requests are made. The XHRToken is more or less an empty object. The XHRToken is passed along to the adapter watch/find methods. The Adapter assigns the XHR to the token. Now the find/watch callsite has a reference to the XHR and can be responsible for cancellation.

Now cancellation is per request rather than per url and our problems go away.

This approach takes inspiration from axios.

[johncowen]

Hey 👋

I really like the sound of this approach. I left a couple of mainly stylistic/design comments. In a similar vein, if these are called 'cancellation tokens' should you cancel them not abort them? You could almost make these CancellationTokens 'not about xhr', probably going too far there though.

None of those comments are important, its mainly just words / bikeshedding but I thought I'd mention in case you like it.

I might have another look at this later, but all looks good!

Oh, I've been looking to see if this effects the connection throttling thing (the 6 connection browser limit thing) but I can't remember where you deal with that, can you remind me where that is?

Thanks,

John

[johncowen]

Wondering if you need to expose xhr as a property here? I've only glanced at the entire changeset for now, but thinking about what you are doing here, I'm guessing you probably want to make the underlying XHR private? No biggie, just incase you want to clean that up a touch. (obvs the this.xhr.abort() below would need to use the _xhr instead)

[DingoEatingFuzz]

Yeah, it's probably not necessary, just seemed like the right thing at the time. However, even making the xhr read-only means you could do something like assign the xhr somewhere else and abort it outside of the token, which is not ideal.

[johncowen]

Usually I think its a good idea to put all defaulting arguments to the right hand side, so either:

reloadRelationship(model, relationshipName, cancellationToken, watch = false) {

or

reloadRelationship(model, relationshipName, watch = false, cancellationToken = {}) {
// or
reloadRelationship(model, relationshipName, watch = false, cancellationToken = new Token()) {

not sure what exactly here, and completely understand this is an improvement so you might be avoiding the argument ordering refactor here.

I 'think' you generally will be providing cancellationTokens so the former possibly makes most sense?

reloadRelationship(model, relationshipName, cancellationToken, watch = false) {

Again no biggie, and I'm thinking you might be leaving this out to avoid the ordering refactor.

[DingoEatingFuzz]

Ha, this was just me being hasty on a Thursday evening 😅 I think I'll change this to make watch and cancellationToken both options, similar to how adapterOptions work.

reloadRelationship(model, relationshipName, options = { cancellationToken: null, watch: false })

[meirish]

Hey @DingoEatingFuzz, this looks good, but I did have one general comment.

Have you thought about what it looks like when cutting jQuery out of the equation? Fetch's AbortController is weird (I think you'd make new Requests, and each has a signal? seems cleaner than a new AbortController for every request...), but I think it would act similarly to how you've done this - you'd just replace the token with the AbortController's signal. Not suggesting you do it now, but something to think about because maybe you do want to do it now? Here's an overview article: https://developers.google.com/web/updates/2017/09/abortable-fetch . ember-fetch(and the underlying polyfill) has support for this now too.

[DingoEatingFuzz]

@johncowen, thank you for the review! To answer your question about the six connection limit, that happens on page transitions. From the willTransition hook, all the watch* ember concurrency tasks are canceled, which then makes the watch properties continue to the finally statement.

[DingoEatingFuzz]

@meirish, yes, I did briefly think about the post-jquery world. AbortControllers are awesome, but they're still quite new and not supported in IE11. As you mentioned, AbortControllers follow a similar token pattern. In fact I may be able to replace the XHRToken class with AbortController, but this is all moot for the time being.

I suspect that even after removing jquery, Nomad will be using XHRs. In which case the adapter will have to change, but everything else will remain the same.

[DingoEatingFuzz]

Okay, I ended up standardizing on Abort instead of Cancel, since that's what both XHR and fetch (ala AbortController) use.

[github-actions]

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.