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

[now-client] Clean up fetch headers #3255

Closed
wants to merge 1 commit into from

Conversation

@rdev
Copy link
Member

rdev commented Nov 6, 2019

No description provided.

@rdev rdev requested a review from styfle Nov 6, 2019
@rdev rdev requested a review from leo as a code owner Nov 6, 2019
@styfle styfle added the now-client label Nov 6, 2019
@styfle styfle changed the title Clean up headers in `now-client` [now-client] Clean up fetch headers Nov 6, 2019
@styfle
styfle approved these changes Nov 6, 2019
Copy link
Member

styfle left a comment

Nice!

@@ -125,6 +125,8 @@ export const fetch = async (
// @ts-ignore
opts.headers.Authorization = `Bearer ${token}`;
// @ts-ignore
opts.headers.accept = `application/json`;

This comment has been minimized.

Copy link
@styfle

styfle Nov 6, 2019

Member

Does the case matter here?

This comment has been minimized.

Copy link
@TooTallNate

TooTallNate Nov 6, 2019

Member

Case doesn't matter for HTTP headers. However we should probably be consistent (consistently lower-case IMO).

This comment has been minimized.

Copy link
@styfle

styfle Nov 6, 2019

Member

I seem to recall there was a bug with Authorization and so it was changed to uppercase 🤔

This comment has been minimized.

Copy link
@TooTallNate

TooTallNate Nov 6, 2019

Member

Then we should fix that on the server-side. If there is a bug, it's with a non-Node.js related server, since the core http server module lowercases incoming headers.

This comment has been minimized.

Copy link
@TooTallNate

TooTallNate Nov 6, 2019

Member

Big 👍 for this change here btw, so that proxy level errors can be properly parsed instead of "Unexpected token…"

@@ -99,7 +99,7 @@ export default async function* upload(
headers: {
'Content-Type': 'application/octet-stream',

This comment has been minimized.

Copy link
@styfle

styfle Nov 6, 2019

Member

It might be good to add 'Content-Length': body.length too

@@ -125,6 +125,8 @@ export const fetch = async (
// @ts-ignore
opts.headers.Authorization = `Bearer ${token}`;
// @ts-ignore
opts.headers.accept = `application/json`;

This comment has been minimized.

Copy link
@TooTallNate

TooTallNate Nov 6, 2019

Member
Suggested change
opts.headers.accept = `application/json`;
opts.headers.accept = 'application/json';
@@ -125,6 +125,8 @@ export const fetch = async (
// @ts-ignore
opts.headers.Authorization = `Bearer ${token}`;

This comment has been minimized.

Copy link
@TooTallNate

TooTallNate Nov 6, 2019

Member

See comment below.

Suggested change
opts.headers.Authorization = `Bearer ${token}`;
opts.headers.authorization = `Bearer ${token}`;
@@ -125,6 +125,8 @@ export const fetch = async (
// @ts-ignore

This comment has been minimized.

Copy link
@TooTallNate

TooTallNate Nov 6, 2019

Member

I wonder if it would be possible to avoid all the @ts-ignore by using object spread:

opts.headers = {
  ...opts.headers,
  authorization: `Bearer ${token}`,
  accept: 'application/json',
  'user-agent': `now-client-v${pkg.version}`
};
@leo leo closed this Nov 11, 2019
@leo leo deleted the fix/now-client-headers branch Nov 11, 2019
@leo leo restored the fix/now-client-headers branch Nov 11, 2019
rdev added a commit that referenced this pull request Nov 11, 2019
kodiakhq bot added a commit that referenced this pull request Nov 11, 2019
Reopen of #3255
@leo leo deleted the fix/now-client-headers branch Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.