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

Body.json reviver #104

Closed
heruan opened this issue Aug 5, 2015 · 18 comments
Closed

Body.json reviver #104

heruan opened this issue Aug 5, 2015 · 18 comments

Comments

@heruan
Copy link

heruan commented Aug 5, 2015

Body.json() should take an optional parameter to be passed to JSON.parse(json[, reviver]) function to use a reviver callback to parse JSON data.

See: JSON.parse(): Using the reviver parameter at the MDN.

heruan pushed a commit to heruan/fetch that referenced this issue Aug 5, 2015
`Body.json()` should take an optional parameter to be passed to `JSON.parse(json[, reviver])` function to use a _reviver_ callback to parse JSON data.

P.S. I understand this is a polyfill, see whatwg/fetch#104
@domenic
Copy link
Member

domenic commented Aug 5, 2015

This doesn't make sense. The whole point of body.json() is that the parsing can happen entirely in C++ without calling back in to JavaScript. If you want to do the parsing in JavaScript, you should just use body.text() and JSON.parse it yourself.

@heruan
Copy link
Author

heruan commented Aug 5, 2015

I understand, I was mislead by the polyfill using JSON.parse(). Why not extend the concept of reviver to the underlying mechanism? This would permit to instantiate JavaScript object from their JSON representation with some logic (e.g. dealing with Dates or returning an existing object instance where the JSON was already parsed before).

@domenic
Copy link
Member

domenic commented Aug 5, 2015

The underlying mechanism is C++. Are you proposing allowing authors to supply C++ code to the browser to execute?

Again, just use .text() and JSON.parse.

@heruan
Copy link
Author

heruan commented Aug 5, 2015

I'm proposing the idea of a way to manipulate the returned object; for example passing a JSON schema to the C++ function which will produce the JavaScript object for validation.
For other uses, I will use Body.text() and JSON.parse().

@annevk
Copy link
Member

annevk commented Aug 6, 2015

What is the processing model that you are proposing?

@jods4
Copy link

jods4 commented Jun 6, 2018

@domenic your answer is dismissing this issue purely on implementation details.

The idea that API .json(reviver) should exist seems appealing to me.

How it is implemented is up to browsers. At "worse", a browser could have a C++ path with no argument and fallback to .text().then(json => JSON.parse(json, reviver)) when there's an argument.

The point is:

  • Body.json() is not consistent with JSON.parse(text[, reviver]). It's the same core function but it doesn't have the same API, which is surprising (in a bad way).
  • JSON being what it is, revivers are quite useful. E.g. to replace strings with Date, since in 2018 automatically serializing dates is still an unsolved problem; or to create typed objects based on $type.

The work-around
fetch("api/hello").then(r => r.text()).then(text => JSON.parse(text, reviver))
is not very pretty, there's no reason that we shouldn't be able to do
fetch("api/hello").then(r => r.json(reviver))

Plus, some browsers might be able to offer more optimized implementations?

An optimized C++ API is not as useful if it's unusable (using .text() instead) or if you have to post-process your object graph after it's been deserialized (which moots the performance win).

@annevk
Copy link
Member

annevk commented Jun 6, 2018

How is it the same core function?

  1. It doesn't share the name.
  2. It operates on a stream of bytes, not a string.
  3. It returns a promise for an object, not an object.

@jods4
Copy link

jods4 commented Jun 6, 2018

@annevk
Both methods are inside the core web platform and both parse serialized json into a JS object.

The key difference is (2) and implies (1) and (3): one operates on a stream of bytes (hence different name and returns a Promise) the other operates on a string.

The fact that their input is different doesn't justify that the API surface of parsing is different.

It's like saying: if you serialize JSON to a file you can choose some properties to omit, but not when you serialize JSON to a memory string where you can transform values, unlike when you serialize JSON to an HTTP body, in which case you can choose to transform camel-case to PascalCase.

Good API design is consistent and predictable.
Wherever the web platform exposes JSON parsing, it should expose the same parsing options.

Unless you consider revivers an obsolete feature? It's rather useful so I hope you don't.

@annevk
Copy link
Member

annevk commented Jun 6, 2018

I don't see why you think it doesn't justify a different API.

@jods4
Copy link

jods4 commented Jun 6, 2018

Same platform feature (JSON parsing), so default should be same/similar API and capabilities, see https://en.wikipedia.org/wiki/Principle_of_least_astonishment
The justification is on you: why a different API?

The only justification I can see in this thread is "code is written in C++ so can't use a JS callback", which is not enough in my opinion. You can have a .json() C++ implementation and .json(reviver) in JS.

Here's some fan-fiction: jods upgrades his old code.
"Hey there's this cool new API fetch to grab data from the server, it even has some deserialization built-in!
I'm gonna replace my ugly XmlHttpRequest code (includes a reviver) with a one liner, so cool."

nope -- can't do. New APIs have a smaller feature-set than old ones.

@annevk
Copy link
Member

annevk commented Jun 6, 2018

The rationale is that the parsing and object allocation can happen in parallel to some extent, which cannot happen if there's callbacks involved.

@jods4
Copy link

jods4 commented Jun 6, 2018

How does that preclude the platform from exposing a compatible API?

function cpp_json(body) { /* native optimised C++ code */ }

Body.prototype.json = function(reviver) {
  return reviver ?
    this.text().then(text => JSON.parse(text, reviver)) : 
    cpp_json(this);
}

There, you have a consistent API.
It has a fast path for users not needing a reviver and an easy to use API for users that do.

People, who need a reviver, will not suddenly not need it anymore just because you don't expose an API for it.
Instead they will get frustrated, not use your function and fallback to something else.

@sliekens
Copy link

+1 for consistent API design. I found this issue when googling how to pass a reviver to json() and this made my eyes roll so hard it hurt

Again, just use .text() and JSON.parse.

The word just should be reserved for solutions that are actually obvious to most people. I would never have thought to use text() because I'm working with objects.

@saas2813
Copy link

saas2813 commented Apr 8, 2019

Actually what is needed is not really the same callback. What is needed is a way to define transformations and Class assignments to do while parsing the JSON string.
Something like:

  • Value matches this RegExp assign this class.
  • Value contains this tag assign this class.
  • Values matching this tag should not be included.
    Basicly an array of
    { matcher: "RegExp", regexp: "...", stdclass:"Date"},
    { matcher: "Tag", tag: "@type", value:"Address", userclass:"Address"}
    { matcher: "Tag", tag: "internal", delete: "true"}

@annevk
Copy link
Member

annevk commented Apr 8, 2019

@saas2813 are there libraries that can perform JSON parsing incrementally (i.e., on a ReadableStream) while doing that which have seen somewhat non-trivial adoption? That'd make a good case for some kind of change to the standard library.

@jeansoulin
Copy link

I found this thread after attempts on stackoverflow. My #1 use of reviver is to reduce the size of the parsed object when working with very huge json files, eg:

(k,v) => (k==="a" || k==="b")? v : undefined; // discarding c, d, ...

It should be done easily on a stream. It is a pity to have to pass through response.text(), or to use the entire object after response.json() before removing all the useless properties. I don't understand why it should be so difficult to implement.

@bergus
Copy link
Contributor

bergus commented Oct 27, 2019

@saas2813 Instantiating a user-supplied class would call its constructor, which is essentially the same as a callback.

@Somnium7
Copy link

Somnium7 commented Aug 2, 2022

Why just don't traverse object after its creation? This way you always use optimized c++ for parsing but can you reviver to convert certain values to Date or BigInt objects. This won't use less memory for big json if someone skips some values, but at least it solves type conversion problem, with consistent syntax.

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

No branches or pull requests

9 participants