Skip to content
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

onOkResponse interceptor #348

Open
1 task done
Amar-Gill opened this issue Jan 2, 2024 · 5 comments
Open
1 task done

onOkResponse interceptor #348

Amar-Gill opened this issue Jan 2, 2024 · 5 comments

Comments

@Amar-Gill
Copy link

Describe the feature

Would anyone think it's helpful to add a new ofetch interceptor? https://github.com/unjs/ofetch#%EF%B8%8F-interceptors

I'm thinking about an onOkResponse interceptor.

Because in my project I have a custom useFetch call that has an onResponseError interceptor as well as an onResponse handler (for successful responses).

Reason is, I must wrap my logic in onResponse handler with if (ctx.response.ok) { ... . In the quest to eliminate boilerplate, I wonder if it would be valuable to add the onOkResponse interceptor, that only runs if ctx.response.ok is true - so the exact opposite of onResponseError which only runs if ctx.response.error is not true.

This idea stemmed from me debugging why my interceptor wouldn't run for a couple hours. The root cause was a silent error in onResponse which blocked onResponseError from running. (Maybe this should be a separate issue).

My onResponse handler depends on the request body to be defined, but during an error response it isn't defined and I got a silent error. Only after wrapping my logic with if (ctx.response.ok) does my onResponseError interceptor run.

Here is a minimal reproduction of the interceptors getting blocked, I think it will work for any other set of interceptors:

const { data } = await useFetch('/api/has-error', {
  onResponse() {
    console.log('On response handler');
    throw new Error('todo');
  },
  onResponseError() {
    // this does not run because error thrown from `onResponse()` interceptor
    console.log('On response error handler');
  },
});

Since an error is thrown from onResponse -- onResponseError will not run.

Additional information

  • Would you be willing to help implement this feature?
@sevillaarvin
Copy link

Thanks for the info. I thought that's how it was supposed to work:

  • onResponse handles response.ok, while
  • onResponseError handles !response.ok

When I tested in my code, with !response.ok, onResponse gets called first, then onResponseError.

@Amar-Gill
Copy link
Author

onResponse will always run, whether the response is an error response or not. I was thinking of adding a separate interceptor for just the response.ok case.

But TBH I think I might be using an anti-pattern in my code:

    onResponse(ctx) {
      if (ctx.response.ok) {
        const projIdOfReport = ctx.response._data.report.projectId;

        activeProject.value = projects.value.find(
          (proj) => proj.id === projIdOfReport
        );
      }
    },

I am accessing the response._data property. My issue was around having to wrap the code in the if (response.ok) -- but I shouldn't be using _data in my code because that is meant for Nuxt internals AFAIK.

@sevillaarvin
Copy link

I am accessing the response._data property. My issue was around having to wrap the code in the if (response.ok) -- but I shouldn't be using _data in my code because that is meant for Nuxt internals AFAIK.

I wouldn't worry about response._data. I got curious and looked into it, and from what I gathered it's just a workaround for node-fetch, but _data is still meant to be used publicly.

Though I agree that response.data would look more pleasing than response._data.

@Amar-Gill
Copy link
Author

Amar-Gill commented Jan 24, 2024

Interesting! Thanks for sharing that context.

So in this case, I do think the proposed enhancement here could be useful. Because to access response._data you must wrap it in if (response.ok) { ... otherwise the interceptor will silently fail when the response is an error.

And a consequence of the silent failure in onResponse is the intended error handler onResponseError does not execute.

Checking that response.ok is truthy is not a big deal for me personally, but I think it would be nice to eliminate boilerplate if we can.

@Smef
Copy link

Smef commented Jun 2, 2024

I think it would be very helpful to handle these separately. A pattern like onSuccess, onError, and onFinish makes a lot of sense to me. onResponse would fire before either response hander (if it stuck around), and onFinish would fire after the other handlers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants