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

Add a request option for custom headers #118

Merged
merged 5 commits into from
May 3, 2024
Merged

Conversation

dlarocque
Copy link
Collaborator

Allow users to pass custom request headers through request options.

Questions:

  • If a user passes a customHeaders object that isn't a Header (e.g. customHeaders: { headerName: 'headerValue'}, rather than customHeaders: new Headers({ headerName: 'headerValue' })), should we a) raise a TypeError: url.requestOptions.customHeaders.entries is not a function, or b) Ignore the custom headers?
  • The original CL adds checks for newlines in apiClient and customHeaders fields. Should these checks be added to this PR, or a separate one?

@hsubox76
Copy link
Collaborator

hsubox76 commented May 1, 2024

  • If a user passes a customHeaders object that isn't a Header (e.g. customHeaders: { headerName: 'headerValue'}, rather than customHeaders: new Headers({ headerName: 'headerValue' })), should we a) raise a TypeError: url.requestOptions.customHeaders.entries is not a function, or b) Ignore the custom headers?

A Typescript user will get a compile-time error right away, but we likely have a lot of non-Typescript users so I think it's worth throwing an error. Probably we should do a more informative one. I'd probably do an instanceof Headers check and then throw a GoogleGenerativeAIError saying customHeaders must be an instance of Headers. We can also allow customHeaders?: Headers | Record<string, string> and if it's not instanceof Headers then try to convert it with new Headers(customHeaders), wrapped in a try/catch where the catch rethrows the message wrapped in GoogleGenerativeAIError and additional text "unable to convert customHeaders value to Headers", if it throws. I'm not sure what kind of errors a failed new Headers(init) throws, if any.

packages/main/src/requests/request.test.ts Outdated Show resolved Hide resolved
packages/main/src/requests/request.ts Outdated Show resolved Hide resolved
@dlarocque dlarocque requested a review from hsubox76 May 2, 2024 20:23
Copy link
Collaborator

@hsubox76 hsubox76 left a comment

Choose a reason for hiding this comment

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

Looks good! Just one more comment, and add a changeset.

packages/main/src/requests/request.ts Outdated Show resolved Hide resolved
@dlarocque dlarocque requested a review from hsubox76 May 2, 2024 20:52
Copy link
Collaborator

@hsubox76 hsubox76 left a comment

Choose a reason for hiding this comment

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

Looks good, just add a changeset.

@dlarocque dlarocque requested a review from hsubox76 May 2, 2024 21:58
@dlarocque dlarocque merged commit 4562366 into main May 3, 2024
3 checks passed
@dlarocque dlarocque deleted the dlarocque/custom-headers branch May 3, 2024 12:37
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