Fix handling of falsy value with the `json` option #223

elijahsmith · 2020-01-16T13:39:43Z

Currently, falsey json values will get mistaken for an empty json value -- this makes the comparison more specific, and will match any json value other than undefined

Fixes #222

currently, falsey json values will get mistaken for an empty json value -- this makes the comparison more specific, and will match any json value other than `undefined`

sindresorhus · 2020-01-16T13:53:58Z

Would you be able to add a test too? So we can be sure to not regress it in the future. https://github.com/sindresorhus/ky/blob/master/test/main.js

sholladay

If this is to be an officially supported use case, I think the documentation for the json option should be updated to clarify that any valid JSON serializable value is supported. Currently it implies that only objects are supported.

Both readme.md and index.d.ts should be updated.

…ion of `json` prop

elijahsmith · 2020-01-17T09:55:15Z

I've changed the implementation just a bit -- now, setting json to anything is functionally equivalent to setting body to the JSON.serialized result of the same anything... including for edge cases like undefined!

I also updated docs to reflect this (set their types to any and updated the descriptions).

Thanks for the feedback!

index.js

szmarczak · 2020-01-17T12:41:06Z

index.d.ts

 	*/
-	json?: unknown;
+	json?: any;


This should be correctly typed. The keys are strings and the values can be: string, number, boolean, null, object, array.

I'm not sure what you mean by "the keys are strings". I've updated the type to explicitly list those types, and included undefined, which is technically allowed (though it serializes to null...)

For TypeScript purposes, I think it should probably stay as unknown.

https://stackoverflow.com/questions/51439843/unknown-vs-any/51439876

szmarczak · 2020-01-17T19:52:22Z

index.d.ts

 	*/
-	json?: unknown;
+	json?: object | Array | string | number | boolean | null | undefined;


undefined is unnecessary here

that's fine... but what's your reasoning? undefined is a valid value to pass the json prop here... that said, this is my first time working with ts so I'm not aware of the subtleties...

sholladay · 2020-01-17T19:36:18Z

index.js

@@ -242,7 +242,7 @@ class Ky {
 			this.request = new globals.Request(new globals.Request(url, this.request), this._options);
 		}

-		if (this._options.json !== undefined) {
+		if (this._options.hasOwnProperty('json')) {


This will make it hard to unset the json option. Also, it will try to stringify and send undefined which will cause JSON.parse() on the server to throw an error.

I think !== undefined is the way to go.

to unset the prop, you'd delete it... the tricky thing here is that setting json is (described as) a "shortcut" for setting .body = JSON.stringify(...), so presumably anything you can pass to JSON.stringify should be able to be passed to json! and, undefined will be stringified to null so the server will be required to parse that (which should be fine).

Having the json prop act ever so slightly different than how it's described is IMHO a recipe for weird edge cases...

undefined will be stringified to null

This is incorrect. JSON.stringify(undefined) returns undefined, which JSON.parse() fails to parse.

sholladay · 2020-01-17T19:53:21Z

readme.md

@@ -139,9 +139,9 @@ Internally, the standard methods (`GET`, `POST`, `PUT`, `PATCH`, `HEAD` and `DEL

 ##### json

-Type: `object`
+Type: `any`


Maybe instead of an actual type (which would be very verbose), we should just say "Any valid JSON value", linking to a reference on MDN or elsewhere that specifies what that is.

this is where I can't contribute, as I have no ts experience. I'm happy to go with whatever y'all decide...

I was speaking only of the documentation for the API in the readme, my comment isn't TypeScript related. The TypeScript definitions in index.d.ts, on the other hand, obviously need to be valid TypeScript.

elijahsmith · 2020-01-17T20:10:09Z

the build is now failing on the test headers › non-existent headers set to undefined are omitted, but I can't see why! it started failing when I changed to using 'json' in this._options. Changes to the docs or .ts files would not trigger tests failures, would they?

sholladay · 2020-01-18T00:47:54Z

index.d.ts

 	*/
-	json?: unknown;
+	json?: object | Array | string | number | boolean | null;


I am going to undo this change because it is unrelated to supporting non-object JSON values and there is some history behind why the TypeScript type is unknown. @sindresorhus made this change and explained it in #133 (comment). As far as I know, there still isn't a great solution for that

We can talk about it in the comments here in this PR or you can open a new issue if you want the TypeScript types to be stricter. For now, I think unknown is sufficient to support the changes in this PR.

I'm 👎 on undoing this. I think Sindre was talking about response, not request. When it comes to parsing response, then yes, you have to cast it on your own because you don't know if it's a string, a number or whatever (one of the types mentioned later in this comment). On the other hand, we don't care about the JSON body as long as it is valid JSON. See, it's valid only when it is string | number | boolean | null | {[key: string]: ...} | Array<...>.

@sindresorhus Can you clarify please?

Apologies, I linked to the wrong comment. I edited and fixed the link.

This just made the option harder to use in practice. JSON.stringify() accepts unknown too. And it didn't account for when your have custom types with a toJSON() method. So all in all, the strict type had very little value.

Apologies, I linked to the wrong comment. I edited and fixed the link.

No problem.

And it didn't account for when your have custom types with a toJSON() method.

Well, what about this:

interface JSONable extends Object { toJSON(): string; }

(not sure if it works, not at home atm)

unknown is the only pragmatic type here. I've tried making it strict before and it just turned out to be an inconvenience to users without much value.

See: #133 (comment)

sholladay

LGTM as-is, but we can wait for Sindre's response in regards to the JSONAble interface (I think it's a good idea if it works as expected).

sindresorhus · 2020-01-19T07:04:00Z

Thanks for fixing this 👌

elijahsmith · 2020-01-20T09:42:02Z

I can open a new ticket for this discussion if you'd rather, but, I'm not completely comfortable with the way this was finally resolved.

With the current fix, given

var foo;  // undefined

there is a functional difference between:

ky.put('/endpoint', {
    body: JSON.serialize(foo)
};

...which will send the body null to the server, and:

ky.put('/endpoint', {
    json: foo
};

...which will send no body at all.

I know this seems like a little thing but, especially with low-level libraries like this, small deviations are surprising and frustrating. Is there a reason not to use 'json' in this._options or Object.hasOwnProperty(this._options, 'json')?

sholladay · 2020-01-20T19:06:32Z

I can open a new ticket for this discussion if you'd rather, but, I'm not completely comfortable with the way this was finally resolved.

I am happy to discuss it further. Let's continue here for now.

there is a functional difference between ...
body: JSON.serialize(foo)
...
json: foo

No, there isn't. You have made some faulty assumptions.

You seem to believe that undefined serializes to null, but as I mentioned here, that is incorrect. In reality, undefined === JSON.stringify(undefined). Both are equivalent: they send no body and the Content-Length is 0, regardless of whether you use the json option or the body option.

Is there a reason not to use 'json' in this._options ... ?

Yes. As I mentioned here, this would make it hard to unset the json option, especially on extended instances. You are assuming that the user can delete options.json to unset it, but that is only true if the user uses ky() directly. Here is an example where that would fail, because the options merger used by ky.extend() wouldn't be able to detect that you delete'd a property, so it would simply think you aren't overriding json.

const myOptions = {
    json : { time : Date.now() }
};
const myKy1 = ky.extend(myOptions);
myKy1.post('/ping');  // sends json

delete myOptions.json;  // does not affect myKy1, as options are already set
const myKy2 = myKy1.extend(myOptions);
myKy2.post('/ping');  // bug: still sends json, even though we intended not to

The above code is not "fixed" by using this._options.json !== undefined, but it does mean you can unset the option by simply setting it to undefined, as shown below.

const myOptions = {
    json : { time : Date.now() }
};
const myKy1 = ky.extend(myOptions);
myKy1.post('/ping');  // sends json

myOptions.json = undefined;  // does not affect myKy1, as options are already set
const myKy2 = myKy1.extend(myOptions);
myKy2.post('/ping');  // does not send json (yay!)

Is there a reason not to use ... Object.hasOwnProperty(this._options, 'json')?

This has the same problems as 'json' in this._options, in that overrides won't work correctly. Beyond that, I don't see a need for a hasOwnProperty check. We spread the options argument into this._options. All properties of this._options (other than the language built-ins) are own properties, including json. Yet it's also not meaningful whether json is an own property or a prototype property.

elijahsmith · 2020-01-20T22:24:24Z

thanks for the followup...

You seem to believe that undefined serializes to null, but as I mentioned here, that is incorrect. In reality, undefined === JSON.stringify(undefined)

this is correct, and I was wrong, I extrapolated incorrectly from JSON.stringify([undefined]) == "[null]", and should have investigated further! I appreciate the thorough response.

make check for json content more specific

bccf1de

currently, falsey json values will get mistaken for an empty json value -- this makes the comparison more specific, and will match any json value other than `undefined`

add a test

0513e97

sholladay changed the title ~~make check for json content more specific~~ Fix handling of falsey json option Jan 16, 2020

sholladay requested changes Jan 16, 2020

View reviewed changes

elijahsmith added 2 commits January 17, 2020 10:52

improve feature parity with setting body to serialized representata…

937aec6

…ion of `json` prop

update docs

e8589bc

linting fix

2adb50b

elijahsmith requested a review from sholladay January 17, 2020 11:09

szmarczak requested changes Jan 17, 2020

View reviewed changes

change implementation, improve docs

fb7901c

szmarczak reviewed Jan 17, 2020

View reviewed changes

sholladay requested changes Jan 17, 2020

View reviewed changes

remove undefined from list of possible values provided to json prop

f1a4ab4

Always ignore undefined in json option

152b0d6

sholladay reviewed Jan 18, 2020

View reviewed changes

sholladay added 3 commits January 17, 2020 19:53

Undo type definition changes

eb39242

See: #133 (comment)

Link to JSON.stringify() documentation

c0863e7

Fix order of actual and expected values in tests

a9e9caa

sholladay approved these changes Jan 19, 2020

View reviewed changes

Update readme.md

0864d16

sindresorhus changed the title ~~Fix handling of falsey json option~~ Fix handling of falsy value with the json option Jan 19, 2020

sindresorhus merged commit b20779b into sindresorhus:master Jan 19, 2020

snyk-bot mentioned this pull request Feb 12, 2020

[Snyk] Upgrade ky from 0.3.0 to 0.16.2 EverlongProject/create-react-app#14

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix handling of falsy value with the `json` option #223

Fix handling of falsy value with the `json` option #223

elijahsmith commented Jan 16, 2020 •

edited by sholladay

Loading

sindresorhus commented Jan 16, 2020

sholladay left a comment

elijahsmith commented Jan 17, 2020

szmarczak Jan 17, 2020

elijahsmith Jan 17, 2020

sholladay Jan 17, 2020

szmarczak Jan 17, 2020

elijahsmith Jan 17, 2020

sholladay Jan 17, 2020

elijahsmith Jan 17, 2020

sholladay Jan 18, 2020

sholladay Jan 17, 2020

elijahsmith Jan 17, 2020

sholladay Jan 17, 2020 •

edited

Loading

elijahsmith commented Jan 17, 2020

sholladay Jan 18, 2020 •

edited

Loading

szmarczak Jan 18, 2020

sholladay Jan 18, 2020 •

edited

Loading

szmarczak Jan 18, 2020

sindresorhus Jan 19, 2020

sholladay left a comment

sindresorhus commented Jan 19, 2020

elijahsmith commented Jan 20, 2020

sholladay commented Jan 20, 2020 •

edited

Loading

elijahsmith commented Jan 20, 2020

Fix handling of falsy value with the json option #223

Fix handling of falsy value with the json option #223

Conversation

elijahsmith commented Jan 16, 2020 • edited by sholladay Loading

sindresorhus commented Jan 16, 2020

sholladay left a comment

Choose a reason for hiding this comment

elijahsmith commented Jan 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sholladay Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

elijahsmith commented Jan 17, 2020

sholladay Jan 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sholladay Jan 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sholladay left a comment

Choose a reason for hiding this comment

sindresorhus commented Jan 19, 2020

elijahsmith commented Jan 20, 2020

sholladay commented Jan 20, 2020 • edited Loading

elijahsmith commented Jan 20, 2020

Fix handling of falsy value with the `json` option #223

Fix handling of falsy value with the `json` option #223

elijahsmith commented Jan 16, 2020 •

edited by sholladay

Loading

sholladay Jan 17, 2020 •

edited

Loading

sholladay Jan 18, 2020 •

edited

Loading

sholladay Jan 18, 2020 •

edited

Loading

sholladay commented Jan 20, 2020 •

edited

Loading