Catching errors from backgroundReload

[nicolasdular]

Let's says we have a UsersIndexRoute where we load all users.

model() {
  return this.store.findAll('users');
}

When we load the page for the first time (hard reload) and get a HTTP 500 error back we get this error in the ApplicationRoute's error() and can render an error page and everything is fine.

But: Let's says we already loaded the Ember App but only loaded a subset of all users, go to the /users page and call findAll() again. Ember immediately gives back the subset of users we already loaded in the store and fetches all the other users in a background request.

Now in our case we get a HTTP 500 error back in the backgroundReload, but it seems like https://github.com/emberjs/data/blob/master/packages/ember-data/lib/system/store.js#L1027 doesn't return the promise array back and we can't catch the error in findAll() anymore, or receiving any error in the ApplicationRoute's error().

My question is: how and where can I catch errors from backgroundReload? Why not returning promiseArray(_findAll(adapter, this, typeClass, sinceToken, options)); ?

[bmac]

Hi @nicolas-dular. I agree that we should add some way to expose errors with background loaded record arrays.

Retuning the promiseArray(_findAll(adapter, this, typeClass, sinceToken, options)); would cause the store.findAll to return a promise that does not resolve right away which is not the intent of shouldBackgroundReloadAll. Maybe we should add an property on the promise array to allow users the access the promise from the _findAll?

cc @igorT @tomdale @wycats

[philipp-spiess]

What I do currently as a workaround is that I override _fetchAll and add a catch clause to the background reload promise which triggers Ember.onBackgroundReloadError - otherwise those errors will go unhandled into Ember.onError.

We have avoided most occurrences by making a smarter shouldReload which knows if all elements are already pushed into the store (via findAll) or just a subset that has been side loaded or whatever. When we now receive a background reload error we can at least say that there's not only a subset of the data in the store (although the store can have changed in that time).

Edit: I would be happy to share my "smart" solution with you if you want to 😄

[bmac]

@philipp-spiess I think something like onBackgroundReloadError is ok for now.

I think the long term solution is likely to involve implementing a general purpose event system for the store so users can react to this error and other events.

[stefanpenner]

closing in-favor of RFC.

[flache]

Are there any updates on this yet? Is there a officially supported solution? I am facing the same issue now and I would assume this scenario is not very uncommon.

[lolmaus]

@stefanpenner Why did you close this?

I believe, the issue should stay open until an implementation PR of the RFC (not the RFC PR) is merged into master.

But currently the RFC does not even exist.

The problem, on the other hand, is very real. The workaround of overriding private API and relying on global events does not feel right either.

Please reopen! 🙇‍♂️

[hjdivad]

@lolmaus you should be able to explore solutions using the REQUEST_SERVICE feature flag. This is obviously experimental API (hence the feature flag) so feedback on ergonomics & utility are of course welcome.

This use case seems squarely in the realm of what the request service ought to enable: if it's a pain to handle this that to me is a clear sign we need to iterate on the request service API.

cc @igorT

[raido]

I’ve hit issue of catching background fetch promises as well. I’ll probably turn off background reloading for “quick fix”.

[igorT]

Assigned to @igorT to validate this works with REQUEST_SERVICE

[runspired]

I have a few thoughts on how to improve this with RFC emberjs/rfcs#860

Since the RFC gives control over catching and handling the error to the request-manager, end users can choose to swallow the error.

This allows us to consider two options:

1. we could allow empty responses and ignore them, if a handler wished to swallow the error for a backgroundReload we would not generate a new error when seeing an empty response, we'd just move on.
2. we could ensure that background requests that reject update the cache but are swallowed at that point (since we now have the full context that it was a background request throughout the system). This prevents consuming applications from being required to catch the error unless they wish to via a handler.

I'm going to leave this feedback on the RFC as something to add to it, and close this in favor.