WIP: better errors

[NullVoxPopuli]

No description provided.

[frederikbosch]

@NullVoxPopuli Is this conform Ember Data conventions? Should we not reject with an error instead of throwing an exception?

[sukima]

The need to have an exception object is for the help of consumers to the addon. Once it hits the console there is a stack trace, There is a clear error message that developers can understand. And as an object a developer can easily catch the error and choose to deal with it (i.e. offer a friendly not found message translated correctly).

As for the difference between reject vs throw these are exactly the same thing. There is no difference. If you throw inside a then it is the same thing as returning a rejected promise. The difference is semantic and seeing a throw is more clear to those who are reading the code then a reject which doesn't stand out and (for me) would require a larger level of cognitive load then a language keyword like throw.

[NullVoxPopuli]

FWIW, @frederikbosch I plan to do a couple subsequent PRs

Typescript,
conversion to async/await

I hope that both of those will help out with readability and conveying intent

[sukima]

For precedent ember-ajax uses this pattern to make things easier for app developers.

[NullVoxPopuli]

Conversation about this addon, this PR, and some weird ember-data behavior w/r/t the test failures:
https://discordapp.com/channels/480462759797063690/486549196837486592/512677986743615498

Not sure on a path forward, honestly.

with ember-localforage-adapter all relationships can be sync.

[frederikbosch]

@NullVoxPopuli I don't have a Discord account. And since I have enough conversation accounts already, I am going to skip that one. What is the basic consensus regarding throwing/rejecting and or what behaviour is expected from Ember Data's perspective?

I also have a question on that. The reason that I did not make the current version a stable one, is that I was doubting whether missing belongsTo relationships should fail or return null. My feeling is that the behaviour in Ember Data might have been changed since the last time I touched this codebase, which was until the beginning of this week more than one or two years I guess.

[NullVoxPopuli]

well, around exception handling, using an exception is the right way to go always, but we learned that ember-data has some super weird behavior around rejected promises, specifically, with relationships.

Here is a twiddle demonstrating the problem https://ember-twiddle.com/47e64fc2de362566b57895061b22b4b5

the current behavior in e-localforage-adapter is FAIL_MODE = OUTCOMES.REJECT;
some of our discussion:

[frederikbosch]

@NullVoxPopuli Ah ok. The intention of the library would be to stay in line with Ember Data. Don't forget that this code originally comes from 2014. I created it to support my app that required it. Myself I am not really up-to-date with all Ember/Ember Data developments. It could be that the addon behaves incorrectly. I did the upgrade recently to upgrade my app, and that went really well. Feel free to come up with better solutions. I am happy to merge them, as long as there are green lights.

[NullVoxPopuli]

@frederikbosch I guess, maybe we need some help determining what intended behavior is?

Say, a record from a relationship does not exist, what should happen:

1. error (this would be brittle for anyone using localforage / offline apps)
2. ignore (current) - but ember-data has different behavior based on whether or not reject is called with undefined or a string. -.-
   • also downside to this, is if you just do a normal findRecord and don't care about relationships, if the record isn't found, you don't get an error (why I added the exception in the first place)
     • just a rejection for an unknown reason
3. something else

[frederikbosch]

@NullVoxPopuli Well, during the upgrade I did find out some things.

1. There is code within Ember Data that is focussing on this topic.
2. That code did fire during the upgrade of this addon to 3.0-betaX for has many relationships, but not for belongsTo. I have no idea why it did not. As my own app was not having troubles by it, I was OK with it. At least, temporarily.
3. So let's accept that missing hasMany relationships will lead to a rejection when fetching the relationship.
4. What to do with belongsTo? Is there any convention within Ember Data? If not, what would be our best guess with regard to offline applications? Or maybe a setting?
5. First let's open an issue here that describes our thinking, then open issue at Ember Data for clarification.

[NullVoxPopuli]

@frederikbosch cool, yeah -- this is certainly strange.

for now, I changed me code:

    try {
      record = await this.store.findRecord('identity', u_id);
    } catch (_error) {
      this.toast.error(this.intl.t('ui.chat.errors.contactNotFound'));
    }

and previously, it was

    try {
      record = await this.store.findRecord('identity', u_id);
    } catch (error) {
      this.toast.error(error);
    }

the downside to this is that I'm assuming that the only reason findRecord can fail is that the record wasn't found.

I assume there could be other errors, like if localforage hit a snag or something.

So, we can close this PR, and work on trying to get some less troll-y behavior in ember-data, perhaps

[sukima]

I think this conversation exposed two issues which are in fact seperate issues.

1. If a relationship is missing the current behavior is to fail silently. This is a byproduct of using return reject() which (for whatever reason) is squashed by ember-data. This behavior is wrong but this addon is dependent on that bug.
2. The findAll attempts to resolve relationships up front (essenially performing {async: false} by default. However, this is unexpected behavior from the perspective of ember-data which is wrapping a synchronous relationship in a asynchronous proxy.

Since both issues provide the consumer with unexpected results (the relationship type is a fulfilled promise proxy resolved synchronously and no clear error for missing relationships) a change in the API is needed. However, a change would introduce breaking changes in current behavior and that is the conclusion of what needs to be discussed. @NullVoxPopuli, that seem about right?

[NullVoxPopuli]

seems about right. good summary 👍
maybe @runspired has ideas on a path forward?
(cause I think the only way really forward is to make some ember-data changes)

[frederikbosch]

@sukima You are correct, in this thread multiple issues are being discussed. I just created 3 separate issues. Feel free to comment on any one of them.

[frederikbosch]

I put descriptions in all three issues, gathered from this thread. Maybe you can help resolve them before we tag 3.0 stable.

[NullVoxPopuli]

thanks for writing those up!