troyanskiy / ngx-resource

Resource (REST) Client for Angular 2
http://troyanskiy.github.io/ngx-resource/
200 stars 46 forks source link

Question: Why is there no 'error' callback? #141

Closed Mysame closed 7 years ago

Mysame commented 7 years ago

It's overkill to constantly have to write

.$observable.subscribe(() => {}, (error) => {/*stuff*/});

I realise the "recommended" approach is to use a ResponseInterceptor, but this has proven to be too high-up to give proper feedback to both the user, and the current component that's making the call.

Are there any plans to simply allow a third argument to a post that handles the onError ?

troyanskiy commented 7 years ago

Hello. Thanks for the issue. I will try to implement that asap.

Mysame commented 7 years ago

@troyanskiy , thank you very much for adding this so very quickly.

However, I'm afraid to report this functionality doesn't work when it's a 'get' http call. This is because in the ResourceMethod default function, there's some trickery to set Callback properly in both post and get calls, but this isn't done for the onError call.

You can see an example here: 2017-10-09_10-55-49

Notice how onError is null even though it's filled as third argument in the args list.

troyanskiy commented 7 years ago

Hello @Mysame. Big changes are coming... I decided to rewrite the lib and that should cover many issues. So please wait a bit, will let you know about new release personally, so you can test it. I hope it will be done this week.

Mysame commented 7 years ago

@troyanskiy Rewrite? Sounds scary. Hopefully it remains stable. Thank you for the update, looking forward to it.

troyanskiy commented 7 years ago

@Mysame I would like to know your opinion. I thinking to make kind of abstract core lib and for now 2 request implementations (with Http from angular/http and HttpClient from angular/common/http) sub libs. Finally, it will have something like main core module where the developer will define which request implementation to use (or develop his own). It will be something like

export function createRequester(http: HttpClient) {
    return new ResourceRequesterHttpClient(http);
}

@NgModule({
    imports: [
        BrowserModule,
        ResourceModule.forRoot({
            requester: { // attribute name will be changed
                provide: ResourceRequester,
                useFactory: (createRequester),
                deps: [HttpClient]
            }
        })
    ],
    bootstrap: [AppComponent]
})
export class AppModule { }

And also I'm thinking to make the library work on the server side (node.js) with an implementation of requester for node.js, so the question is should I get rid of Observables from core lib in order to not install RxJs on the server side.

What do you think about that (get rid of observables)?

Mysame commented 7 years ago

Definitely do not get rid of observables, too many other implementations depend on it. Relevant example would be a custom reactive form validator, which relies on getting returned an observable.

Rewriting bad parts is fine, but what you're suggesting leans closer to a different (new) library alltogether. It would break too much for the current users, I fear.

troyanskiy commented 7 years ago

@Mysame Do you have really good reason to use Observables for the http requests in general? In my own opinion Observables is really good for something continuous, while http request it's kind of single fire Observable which is kind of Promise.

Personally, I switched all my projects to async/await Promise based resource.

async loadData() {
  try {
    this.data = await this.myResource.load();
  } catch (err) {
    // do something with error (Response)
  }
}

And if somebody really needs Observable, then it can be created from Promise

Mysame commented 7 years ago

A promise does make more sense, I agree. However, Observable is used per design by the Angular team. But since, as you mentioned, you can create an observable around a promise, this is a non-issue.

rickdoesdev commented 7 years ago

@Mysame

However, I'm afraid to report this functionality doesn't work when it's a 'get' http call. This is because in the ResourceMethod default function, there's some trickery to set Callback properly in both post and get calls, but this isn't done for the onError call.

After a lot of fiddling; I've gotten around this particular issue by declaring ResourceMethodStrict with an empty second paramater which ensures there's always 4 values going into the resourceAction call so the onError fires. It's kind of hacky, but it feels like it's working? TBH I'm not sure what ResourceMethodStrict is meant to do so repurposing it this way might cause me headaches later.

@ResourceAction({
    isArray: false,
    auth: true,
    path: '/{!id}'
  })
  getMyResource: ResourceMethodStrict<{ id: any },{}, IMyResource>;

On error this actually triggers the onError callback since the arguments array is right size.

return this.myService.getMyResource({ id}, { id}, (res)=>{}, (err)=>{});

@troyanskiy

Making the GET request work correctly with onError and ResourceMethod instead of abusing ResourceMethodStrict is not hard; I'll make a PR if you would like to review it.

troyanskiy commented 7 years ago

Hello all. Please retest the issue with the new version.

Thanks.

troyanskiy commented 7 years ago

Hello all.

I've released some kind of beta of new library which is called rest-core + rest-ngx. Please check them rest-core and rest-ngx.

It works the way like ngx-resource but has a lot of breaking changes. Something was removed or simplified.

I've migrated all my projects to the new library. If you wish to switch to HttpClient or use some other http handlers like fetch try to migrate your projects to the lib.

Bugs and help requests are welcome in corresponding repos.

Thanks!