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

Set accept header for ky shortcut methods #118

Merged
merged 3 commits into from Apr 19, 2019

Conversation

Projects
None yet
4 participants
@selrond
Copy link
Contributor

commented Apr 3, 2019

closes #83

I was first thinking about conditionally setting this inside kys constructor, but it would mean more complexity, as there's no way to know whether the method is set by calling shortcut method or not. We get this for free in createInstance function, where shortcut methods are generated, and where I eventually decided to set the header.

@@ -41,3 +41,21 @@ test('custom method remains identical', async t => {

await server.close();
});

test('accept header for method shortcuts is application/json', async t => {
t.plan(6);

This comment has been minimized.

Copy link
@szmarczak

szmarczak Apr 3, 2019

Collaborator

No need to use this

This comment has been minimized.

Copy link
@selrond

selrond Apr 10, 2019

Author Contributor
index.js Outdated
const {headers: providedHeaders, ...restOfOptions} = options;
const headers = new Headers(providedHeaders);
if (!headers.has('Accept')) {
headers.set('Accept', 'application/json');

This comment has been minimized.

Copy link
@szmarczak

szmarczak Apr 3, 2019

Collaborator

This is inappropriate - it should be set only if someone does ky(...).json().

This comment has been minimized.

Copy link
@selrond

selrond Apr 3, 2019

Author Contributor

@szmarczak I've explicitly asked about this here and you confirmed it.
Besides, i don't believe it's technically possible to set accept headers once json() is called on the return value from ky(...) - the request has already been made right?

This comment has been minimized.

Copy link
@szmarczak

szmarczak Apr 3, 2019

Collaborator

I have missed this part:

there needs to be Accept: application/json header set by default.

Anyway as you can read from the OP:

One thing that got does, which I think it awesome, and which we don't have in ky, is setting the accept header when .json() is called.

So yeah, we need to iterate over every shortcut method and just replace .json() I think.

@sholladay

This comment has been minimized.

Copy link
Collaborator

commented Apr 3, 2019

Sorry I didn't comment on the issue about this earlier. The expected behavior is for the Accept header to be set based on the responseType, not the requestMethod.

In got, this is accomplished by having .json() modify the options object used by the main function.

@szmarczak

This comment has been minimized.

Copy link
Collaborator

commented Apr 3, 2019

Hmm... As @selrond did already notice replacing .json() won't have any effect because the request has been made.

One of the solutions would be to cancel the previous request and make a new one OR delay making the request.

@sholladay

This comment has been minimized.

Copy link
Collaborator

commented Apr 3, 2019

I haven't looked much into how got deals with that part of it. Does it delay the request? If so, how long is it delayed for, one tick of the event loop?

@szmarczak

This comment has been minimized.

Copy link
Collaborator

commented Apr 4, 2019

Yup. It delays the request using setImmediate.

@sholladay

This comment has been minimized.

Copy link
Collaborator

commented Apr 4, 2019

Delaying the request seems fine to me, albeit it's a little strange.

Alternatively, we could change the structure of the API to be await ky.get.json(input, options) instead of await ky.get(input, options).json(). That way Ky knows how to parse the response before the request is sent.

@selrond

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

@sholladay Delaying the request for the sake of arbitrary API shape seems totally unnacceptable IMHO.

@sholladay

This comment has been minimized.

Copy link
Collaborator

commented Apr 5, 2019

It wouldn't be delayed by any meaningful amount of time. We would just do a simple setTimeout(fn, 0). I agree that is not ideal and it feels a bit icky, but I also don't think it poses any material problem for users. It seems that got has been getting by just fine with this approach.

That said, the delay wouldn't be necessary with my proposed API. So thoughts on await ky.get.json(input, options)?

@szmarczak

This comment has been minimized.

Copy link
Collaborator

commented Apr 6, 2019

I vote for the await ky(input, options).json() approach.
This way we're staying familiar with the fetch API.

The delay would be <=1ms, not a significant change.

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Apr 9, 2019

I vote for the await ky(input, options).json() approach.

👍 Me too.

The delay would be <=1ms, not a significant change.

Yup, it would be neglible.

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Apr 9, 2019

This behavior should also be documented in the readme.

@selrond

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

@szmarczak @sholladay I've been going through got source, but haven't found anything that'd suggest me it's setting accept header when got().json() is called.
The only thing I found is this: https://github.com/sindresorhus/got/blob/b5e443b2f9/source/request-as-event-emitter.ts#L350

which suggest, that only checks for options.responseType. Could you please point me to the right direction?

@szmarczak

This comment has been minimized.

Copy link
Collaborator

commented Apr 10, 2019

Place await delay(1); before

ky/index.js

Line 206 in 7114ba3

let response = await this._fetch();

Then I would replace

ky/index.js

Lines 66 to 72 in 7114ba3

const responseTypes = [
'json',
'text',
'formData',
'arrayBuffer',
'blob'
];

with

const responseTypes = {
	json: 'application/json',
	text: 'text/*',
	formData: 'multipart/form-data',
	arrayBuffer: '*/*',
	blob: '*/*'
};

and replace

ky/index.js

Lines 227 to 231 in 7114ba3

for (const type of responseTypes) {
result[type] = async () => {
return (await result).clone()[type]();
};
}

with

 for (const [type, mimeType] of Object.entries(responseTypes)) { 
 	result[type] = async () => { 
                headers.set('accept', mimeType);
 		return (await result).clone()[type](); 
 	}; 
 } 

That's all. Remember to test this :)

@selrond

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

@szmarczak @sholladay pls review again

Concerning the simple test, all other methods behave exactly like .text(), but would require correct data to be sent back, which would in case of multipart/form-data require additional library like https://github.com/form-data/form-data, so I decided to leave all others out.

Besides, I'm a bit concerned with this type of usage:

const tmp = await ky.get(..)
/* other stuff */
const result = await tmp.json() // headers are going to be pointlessly set here in `json()` call, as tmp already contains promise of result of request that's already been fired

The only way it'd work is when one uses ky.<requestMethod>(..).<responseType>() together. Isn't it too hacky?

Show resolved Hide resolved readme.md Outdated
Improve wording
Co-Authored-By: selrond <sebastian.andil@gmail.com>

@sindresorhus sindresorhus merged commit afc18ea into sindresorhus:master Apr 19, 2019

4 checks passed

Travis CI - Pull Request Build Passed
Details
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to 7114ba3
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@selrond selrond deleted the selrond:feature/set-correct-accept-header-on-shortcut-methods branch Apr 19, 2019

szmarczak added a commit to dangdennis/ky that referenced this pull request Apr 21, 2019

Set accept header for ky shortcut methods (sindresorhus#118)
Co-Authored-By: Szymon Marczak <36894700+szmarczak@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.