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.

Sign up for GitHub

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

Jump to bottom

Add Jsonifiable type #492

Merged
merged 14 commits into from
Nov 5, 2022
Merged

Add Jsonifiable type #492

merged 14 commits into from
Nov 5, 2022

Conversation

itsjohncs
Copy link
Contributor

Closes #356.

@itsjohncs
Copy link
Contributor Author

Whoops, I somehow missed that undefined being allowed is desired! Admittedly I did a pretty cursory reading of #356. I'll update the type to permit undefined.

@itsjohncs
toJSON must return a JsonValue, not Jsonifiable.
d8d78f8 
I didn't realize stringify will not recursively call `toJSON` on
objects returned from `toJSON`.
@itsjohncs
Allow undefined object fields.
d460b37
@itsjohncs
Factor out JsonifiablePrimitive.
8891e51 
I realized that `JsonifiableObject` is more appropriately named to
accept *objects* with a `toJSON` method.
@skarab42 skarab42 mentioned this pull request Oct 20, 2022
Open
@sindresorhus sindresorhus requested a review from voxpelli October 21, 2022 19:02
sindresorhus
sindresorhus requested changes Oct 21, 2022
View reviewed changes
sindresorhus and others added 9 commits October 22, 2022 02:10
@sindresorhus
Update jsonifiable.d.ts
adaea49
@itsjohncs
Don't hard wrap.
78facd8
@itsjohncs
Explain why undefined is allowed.
3ea1177
@itsjohncs
Allow const arrays.
bf6b339
@itsjohncs
Add test for deeply nested toJSON object.
5897f04
@itsjohncs
Add quick common use-case description.
4872d1a
@itsjohncs
Allow toJSON to return any Jsonifiable.
f25dd54 
A previous experiment I did erroneously made me think that toJSON could
not be called on any subobjects that toJSON returned. But there was
clearly something wrong with my experiment because the object in the
new test case is serialized fine by `JSON.stringify`.
@itsjohncs
Fix lint and test failures from last commit.
8b240eb
@sindresorhus
Update jsonifiable.d.ts
f331062
@sindresorhus sindresorhus changed the title Add Jsonifiable type. Add Jsonifiable type Oct 22, 2022
@sindresorhus sindresorhus requested a review from skarab42 November 4, 2022 06:47
@sindresorhus sindresorhus merged commit e11ab80 into sindresorhus:main Nov 5, 2022
@MichaelDeBoey
Copy link
Contributor

Should we use this type in Jsonify as well?

@skarab42
Copy link
Collaborator

skarab42 commented Nov 5, 2022 •
edited
Loading

Should we use this type in Jsonify as well?

No

  • Jsonifiable is used to type values that you expect to pass to JSON.stringify() where { foo: undefined } and plain {} are functionally equivalent.
  • Jsonify is used to type parsed JSON value, coming from eg. JSON.parse() where undefined as a value is impossible.

FI #471 (comment)

@shaharmor
Copy link

shaharmor commented Jun 27, 2023 •
edited
Loading

@skarab42 why not also export JsonifiableObject and JsonifiableArray?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sindresorhus sindresorhus sindresorhus approved these changes

@skarab42 skarab42 skarab42 approved these changes

@voxpelli voxpelli Awaiting requested review from voxpelli

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Proposal: Jsonifyable
5 participants
@itsjohncs @MichaelDeBoey @skarab42 @shaharmor @sindresorhus