-
-
Notifications
You must be signed in to change notification settings - Fork 532
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
Jsonify: handle undefined in array. #310
Jsonify: handle undefined in array. #310
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor suggestion for the test, other than that everything looks good.
test-d/jsonify.ts
Outdated
declare const parsedStringifiedArrayWithUndefined1: Jsonify<[undefined]>; // = JSON.parse(JSON.stringify([undefined])); | ||
expectType<null[]>(parsedStringifiedArrayWithUndefined1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's expecting null[]
, I think it should test Jsonify<undefined[]>
for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what you mean.
I am verifying Jsonify<undefined[]>
is the expected null[]
.
expectType<Jsonify<undefined[]>(parsedStringifiedArrayWithUndefined1);
would be true if the implementation of Jsonify
was wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am verifying
Jsonify<undefined[]>
is the expectednull[]
.
I am saying that it should do that. However, it currently doesn't test Jsonify<undefined[]>
, it tests Jsonify<[undefined]>
.
Ultimately it isn't a big deal, I just didn't see any point in using a tuple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah - I get what you're saying now. Thanks. I will make that correction when I get back to this problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 68474d1
Note that #312 was merged. |
It's unclear to me whether this is still needed after #312? |
I think this is still relevant. It handles undefined in array correctly. See #309 where it was noted that "undefined in an array becomes null". I am sorry this sat in draft. #312 does solve the other part of #309. https://github.com/sindresorhus/type-fest/pull/310/files#r743986229 has a small comment about the tests. I tested |
@sindresorhus - Updated and ready for review. |
Fixes #309
@bbrk24 - Please review and suggest tests if you see anything I missed.
I did not address
Jsonify<{ x?: string }>
. This seems to be a harder problem.I experimented with
And adding
JsonifyObject<T
toJsonify<T>
...But as I started to write tests, I realized I have an issue with
JsonObject
's implementationI feel #65 had the right idea and should allow optional key-value pairs, but not a key-value pair where the value is
undefined
.I need to come back to this... but wanted to leave the above notes if someone else wants to take a crack at it.