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.

Sign up for GitHub

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

Jump to bottom

fix(makeFetcher): include requestTransformer headers #48

Merged
merged 1 commit into from
Jan 31, 2024
Merged

fix(makeFetcher): include requestTransformer headers #48

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ function makeFetcher(baseURL: string | URL, baseOptions: BaseOptions = {}) {
...ri,
headers: mergeHeaders(
typeof headers === 'function' ? await headers() : headers ?? {},
ri.headers ?? {},
requestInit?.headers ?? {},
Comment on lines 119 to 121
Copy link
Contributor Author

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.

  1. Create make-service headers
  2. Request Transform headers
  3. User request headers

or

  1. Create make-service headers
  2. User request headers
  3. Request Transform headers

Copy link
Owner

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hehe, can't remember 😅

Copy link
Owner

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.

Copy link
Contributor Author

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,
});

Copy link
Owner

@gustavoguichard gustavoguichard Jan 30, 2024 •
edited
Loading

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 ;)

Copy link
Contributor Author

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.

  1. Create make-service headers
  2. Request Transform headers
  3. User request headers

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly!

),
})
Expand Down
Loading