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

Allow having interceptos from both defaults and per request #319

Closed
1 task done
enkot opened this issue Nov 11, 2023 · 5 comments
Closed
1 task done

Allow having interceptos from both defaults and per request #319

enkot opened this issue Nov 11, 2023 · 5 comments
Labels
discussion enhancement New feature or request

Comments

@enkot
Copy link

enkot commented Nov 11, 2023

Describe the feature

I need to log some data on each request/response, transform the request url (e.g. /pets/{petId} -> /pets/1) etc.
If I add onRequest to the $fetch.create and then add onRequest to the actual client:

const $myFetch = $fetch.create({
  onRequest() {
     // Log data
  }
})

$myFetch({
  onRequest() {} // logging is lost
})

handler from $fetch.create will be replaced by the handler from $myFetch.

To get around this we need to create the wrapper around $fetch:

function $myFetch(url: string, options: FetchOptions) {
  return $fetch(url, {
    ...options,
    onRequest(ctx) {
      // Log data and transform url
      // ...
      return options.onRequest?.(ctx)
    }
  })
}

What do you think about adding some sort of interceptors "merging strategy" option?

It can be done based on the idea of this PR #353.
If we treat interceptors as an array, we can allow merging arrays from $fetch and $fetch.create. So that interceptors from $fetch and $fetch.create can be enforced to append or prepend to the resulting array of interceptors (similar to Nuxt plugins):

const $myFetch = $fetch.create({
  onRequest: [
    (ctx) => { /* Handler 1 */ }, // same as "enforce: 'default'"
    {
      enforce: 'post',
      handler: (ctx) => { /* Handler 2 */ }
    }
  ]
})

$myFetch({
  onRequest: [
    // Will be appended 
    (ctx) => { /* Handler 3 */ },
    // If you need to prepend in some scenarios
    {
      enforce: 'pre',
      handler: (ctx) => { /* Handler 4 */ }
    }
  ]
})

// Interceptors will be executed in this order:
/*
Handler 4
Handler 1
Handler 3
Handler 2
*/

Alternative Original idea

const $myFetch = $fetch.create({
  onRequest: {
    strategy: 'before', // 'overwrite' | 'before' | 'after' | 'manual', 'overwrite' by default
    handler(ctx) { ... }
  }
})

$myFetch({
  onRequest() {...}, 
})

so that interceptors from $myFetch are called after interceptors from $fetch.create.

Additional information

  • Would you be willing to help implement this feature?
@enkot enkot changed the title Add a strategy to call $fetch() interceptors after $fetch.create() interceptors Strategy to call $fetch() interceptors after $fetch.create() interceptors Nov 11, 2023
@pi0 pi0 changed the title Strategy to call $fetch() interceptors after $fetch.create() interceptors Allow having interceptos from both defaults and per request Nov 12, 2023
@pi0 pi0 added enhancement New feature or request discussion labels Nov 12, 2023
@enkot
Copy link
Author

enkot commented Nov 14, 2023

@pi0
To not introduce breaking changes and because of this proposal we can't "merge" interceptors automatically.
I think the ideal solution is to let the user decide what to return, what the order should be, etc.:

Maybe:

$fetch.create({
  interceptorsStrategy: 'manual', // 'manual' | 'overwrite' 
  async onResponse(ctx){
    const data = await ctx.options.onResponse?.(ctx)
    // do something with data here
    return data
  }
})

or:

$fetch.create({
  onResponse: {
    strategy: 'manual', // 'manual' | 'overwrite' 
    // or
    manual: true,
    async handler(ctx) {
      const data = await ctx.options.onResponse?.(ctx)
      // do something with data here
      return data
    }
  }
})

or more "futuristic" - interceptor prefixed with $ is not overwritten and can control "per request" one:

$fetch.create({
  async $onResponse(ctx){
    const data = await ctx.options.onResponse?.(ctx)
    // do something with data here
    return data
  }
})

@pi0
Copy link
Member

pi0 commented Nov 15, 2023

That proposal did not go through but even if we do that, we shall respect first handled value from interceptors.

@enkot
Copy link
Author

enkot commented Nov 16, 2023

100%, can we just pass handled value as second argument? Do you mean this, right?

$myFetch({
  onResponse(ctx, data) { ... },
  onResponseError(ctx, error) { ... }
})

But for me the main question - should we automatically run interceptors one by one based on the specified strategy or just run the "default" interceptor (from $fetch.create) and let it decide what to do with the "request" interceptor?

Maybe best of both worlds? - overwrite, before, after and manual, so manual means that the default interceptor decides how to run the request's interceptor and if run it at all.

@enkot
Copy link
Author

enkot commented Dec 11, 2023

@pi0 Do you have any thoughts on the API?

@intellild
Copy link

I recommand middleware style

onRequest(context, next) {
  // logic
  next(); // if next is not called, it qeuals to override
}

user can optional run super logic before or after, or at the middle of logic
run at begin:

onRequest(context, next) {  
  next();

  // logic

}

run at end:

onRequest(context, next) { 
  // logic

  next();
}

run at middle:

onRequest(context, next) { 
  // some logic

  next();

  // other logic
}

@enkot enkot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants