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

feat: add onRequest hook to createAPIClient #1469

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

JimTacobs
Copy link
Contributor

@JimTacobs JimTacobs commented Nov 20, 2024

Description

Allow for usage of the onRequest interceptor that's provided by ofetch. Added onRequest to the ApiClientHooks and applied it in a similar way that was done for other hooks.

Added four testcases to make sure it works in the way you'd expect when using this hook.

Added a bit of info to the readme to make sure people know they can use it.

Type of change

New feature (non-breaking change which adds functionality)

ToDo's

None

  • Changeset file provided read more
  • Documentation added/updated
  • Unit-Tests added/updated

Copy link

vercel bot commented Nov 20, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
shopware-frontends-docs ✅ Ready (Inspect) Visit Preview Dec 17, 2024 9:07am

@JimTacobs JimTacobs changed the title Feat/add on request hook feat: add on request hook Nov 20, 2024
@JimTacobs JimTacobs changed the title feat: add on request hook feat: add onRequest hook to createAPIClient Nov 20, 2024
@patzick
Copy link
Collaborator

patzick commented Nov 25, 2024

I wouldn't encourage people to add custom headers via the hook, rather use global headers or headers option in request. I see the usecase for analytics purpose as an example. When would you feel you need to modify headers with this hook?

@JimTacobs
Copy link
Contributor Author

@patzick

The source of this use case is similar to the timeout PR: it’s something we could easily do with the old axios client but found less straightforward with the new ofetch implementation.

In our previous HTTP client, we could globally modify headers dynamically based on the context, like so:

const axiosInstance = swInstance._axiosInstance;

if (callType === CallType.SERVER && forwardedFor) {
  axiosInstance.defaults.headers.common['X-Forwarded-For'] = forwardedFor;
}

if (publicRuntimeConfig.userAgent && callType === CallType.SERVER) {
  axiosInstance.defaults.headers.common['User-Agent'] = publicRuntimeConfig.userAgent;
}

With the new ofetch client, I wasn’t able to find a straightforward way to achieve similar behavior. The strongest use case for the onRequest hook is that it allows us to configure headers once when initializing the HTTP client and have them applied dynamically for every request. This avoids the need to manually set headers in each request or implement redundant wrapper logic, keeping our code cleaner and more maintainable.

For example, setting X-Forwarded-For or User-Agent headers dynamically based on the request’s context can be achieved through the hook without needing per-request configuration. This ensures a centralized and consistent approach to managing such headers.

That said, I’d love to hear your thoughts. Do you think there’s a better way to dynamically modify headers in a centralized manner without adding logic to individual requests? If using hooks isn’t the ideal method, I’m happy to explore other options based on your input!

Copy link

pkg-pr-new bot commented Dec 17, 2024

Open in Stackblitz

@shopware/api-client

npm i https://pkg.pr.new/shopware/frontends/@shopware/api-client@1469

@shopware-pwa/cms-base

npm i https://pkg.pr.new/shopware/frontends/@shopware-pwa/cms-base@1469

@shopware-pwa/helpers-next

npm i https://pkg.pr.new/shopware/frontends/@shopware-pwa/helpers-next@1469

@shopware-pwa/nuxt3-module

npm i https://pkg.pr.new/shopware/frontends/@shopware-pwa/nuxt3-module@1469

@shopware-pwa/composables-next

npm i https://pkg.pr.new/shopware/frontends/@shopware-pwa/composables-next@1469

@shopware/api-gen

npm i https://pkg.pr.new/shopware/frontends/@shopware/api-gen@1469

commit: 6e631d6

Copy link

codspeed-hq bot commented Dec 17, 2024

CodSpeed Performance Report

Merging #1469 will not alter performance

Comparing JimTacobs:feat/add-on-request-hook (6e631d6) with main (87610c0)

Summary

✅ 9 untouched benchmarks

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

Successfully merging this pull request may close these issues.

2 participants