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

Accept non-JSON types with .toJSON() method in Jsonify type #257

Merged
merged 6 commits into from
Sep 10, 2021

Conversation

darcyparker
Copy link
Contributor

@darcyparker darcyparker commented Aug 30, 2021

Fixes #256

@leosuncin pointed out the desire to Jsonify non-JSON that is Jsonable via JSON.parse(JSON.stringify(v)). The example provided was an interface Product that was clearly not assignable to JsonValue because it contains Date values. But Date implements a toJSON() method so the non-JSON value can be converted to JSON in a predictable way.

This pull request updates Jsonify to also transform non-JSON values that are Jsonable. Where Jsonable means the non-JSON value implements .toJSON() method that returns a value that is assignable to JsonValue.

In the tests, parsedStringifiedX uses a non-JSON type that currently exists in the tests and is similar to the example @leosuncin provided. As well, I added a couple of classes of non-JSON objects. (One of the non JSON objects is Jsonable and the other is not)

Some items to reflect on and possibly discuss in this review:

  • Should Jsonify strictly apply to JSON values only? Or is it acceptable to include non-JSON values that are Jsonable with toJSON()?
    • This pull request takes the view that it is acceptable to be more flexible and allow non-JSON values that are Jsonable.
    • Another choice is to keep Jsonify as is... and have a new JsonifyJsonable type to handle non-JSON that is Jsonable.
      • But I think having 2 types would be confusing... and I can't think of a use case where it would be necessary to have 2 types.
      • Can others think of a use case where it is helpful to have a separate JsonifyJsonable type? (This is my only doubt because as I approached thinking about Jsonify recursive cast properties of Date instead of cast to string #256 my initial feeling was that it should be a separate type... I can't create a strong argument for it though. And I am leaning towards relaxing Jsonify to also support non-Json but Jsonable input.)

@leosuncin
Copy link

Maybe call the new type as ToJsonify because it will transform the types

@darcyparker
Copy link
Contributor Author

darcyparker commented Aug 30, 2021

@leosuncin - Is this the name you're suggesting if the existing Jsonify goes unchanged and ToJsonify is an alternate name to JsonifyJsonable that I proposed? And by suggesting this name, do you favour two separate types? Or is the revised Jsonify okay with you?

I am hoping people will like be amicable to a single Jsonify definition that handles:

  1. transforming JSON interface to a type that is assignable to JsonValue
  2. transforming non JSON interface that is Jsonable to a type that is assignable to JsonValue. Where Jsonable means the non JSON value implements .toJSON() method that returns a value that is assignable to JsonValue.

If we agree to a single Jsonify definition, then we don't need to think about the right name for JsonifyJsonable.

But if we really need to separate handling the 2 cases into separate methods, then we need to think about a good name. I am not sure about ToJsonify... It does hint at the Jsonable criteria of having a toJSON() method. But I find it hard to read/parse. JsonifyJsonable is easier to read for me because it communicates JSON can be created from it. If we do need 2 separate methods, I am curious to see what others think the name should be. I am open to opinions.

@sindresorhus
Copy link
Owner

I think it makes sense to support anything with a .toJSON() method, but the docs needs to be updated to make it clear how it works. If someone wants the previous strict behavior, they can complain, and we can then consider a strict version or something of this.

@sindresorhus
Copy link
Owner

// @BendingBender In case you have any opinions. Feel free to ignore.

@BendingBender
Copy link
Contributor

I don't have an opinion on this.

@darcyparker
Copy link
Contributor Author

@sindresorhus - Please review the updated documentation.

@sindresorhus sindresorhus changed the title Update Jsonify type: Adds ability to Jsonify Non JSON that is Jsonable with toJSON() Accept non-JSON types with .toJSON() method in Jsonify type Sep 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jsonify recursive cast properties of Date instead of cast to string
4 participants