-
Notifications
You must be signed in to change notification settings - Fork 4
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
fix(makeFetcher): include requestTransformer headers #48
fix(makeFetcher): include requestTransformer headers #48
Conversation
typeof headers === 'function' ? await headers() : headers ?? {}, | ||
ri.headers ?? {}, | ||
requestInit?.headers ?? {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should decide on a better order.
- Create make-service headers
- Request Transform headers
- User request headers
or
- Create make-service headers
- User request headers
- Request Transform headers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think @danielweinmann suggested it the other day and I can't remember why we didn't add it, do you remember, Dani?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hehe, can't remember 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iamandrewluca just to clarify, why do you need to set headers in the requestTransformer, can't it be done in the headers argument?
const service = makeService(BASE_URL, { headers: async () => { ... } })
I think the reason why we didn't allow it the other day is bc we couldn't figure out the better order and virtually anything could be possible through the headers
function, I'd love to know if you found a blocker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to change the headers based on the request info. For example, to handle that FormData header
const requestTransformer: RequestTransformer = (request) => {
const headers = new Headers(request.headers);
if (!(request.body instanceof FormData)) {
headers.set('Content-Type', 'application/json');
}
return { ...request, headers };
};
const headers = {
Authorization: `Bearer ${session.getIdToken().getJwtToken()}`,
};
const service = makeService(import.meta.env.VITE_API_ORIGIN, {
requestTransformer,
headers,
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case I think it should have precedence over the baseOptions
headers as it has more information to work with and can potentially be used for more specific decisions, WDYT?
If you agree and can make the change I'm good with this PR ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the current order (in PR) is ok 🤔 as the user still has control to override any headers if needed.
- Create make-service headers
- Request Transform headers
- User request headers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly!
@all-contributors please add @iamandrewluca for code, promotion, maintenance, bug, ideas. |
I've put up a pull request to add @iamandrewluca! 🎉 |
I tried to use
requestTransformer
to add some custom headers, but it seems those headers are ignored. This PR should fix it.