-
Notifications
You must be signed in to change notification settings - Fork 135
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
The myth of JSON-serializable object #307
Comments
Pinged public-script-coord: |
We require JSON-serializable object to avoid this: var obj = {foo: null};
obj.foo = obj; This is valid JavaScript, but cannot be serialized into a JSON string. Serialization is necessary for communication to payment apps. Android apps can't receive JavaScript objects, so they take in a JSON string instead. |
The goal here was broadly an author requirement for payment apps. Developers should not expect types that can't be readily serialised to work correctly here. I welcome any language to clarify this but it isn't something to lose too much sleep over. |
It's something to lose sleep over because this is a requirement on user agents (they are supposed to reject non-json-serializable cases) but nothing defines what "json-serializable" really means, which means there can't be an actual implementation of the spec as written. As an implementor I would raise a formal objection to this whole bit. As a test writer, I'm quite certain I would be able to create tests that show that UAs don't implement this part of the spec interoperably (which would mean the spec cannot proceed to REC, per the two interoperable implementations criteria). Please define what's actually supposed to happen. |
I'm trying to understand the fundamental difference of opinion here, please bear with me. Is there a concern that Or is the concern that, despite being precisely specified, the definition is complex and so it's hard to get a conforming implementation in this context (and so existing implementations likely already don't conform to that definition)? FWIW the blink implementation appears to rely just on our JSON.stringify method succeeding. So the main opportunities I'd see for this not to conform to the above rough definition are:
So we definitely have conformance bugs in blink ( Perhaps the primary issue for the WG here is conformance testing? If the definition is written is in fact mostly OK, how should we thoroughly test it such that bugs like blink's would be caught? Could we leverage the test262 JSON tests at all? |
Or perhaps it would be simpler to just precisely define a validation along the lines of this: The value must satisfy:
I'm guessing we might end up with such an implementation in blink, so perhaps making a definition that's somewhat redundant with the JSON spec (and so requiring a dedicated test suite separate from test262) is the more practical option? |
No. But note that this spec never invokes JSON.parse/stringify. See also #338
It really depends on your definition of "loss of data".
Maybe. See thread starting at https://lists.w3.org/Archives/Public/public-script-coord/2016OctDec/0071.html if you haven't seen it yet.
Note that invoking JSON.stringify has side-effects, so if the intent is that it be invoked then the spec has to say exactly when that happens. The spec clearly doesn't do that. My post in the above linked public-script-coord thread attempts to outline a partial (necessary by not sufficient) test for a side-effect-free definition of "no loss of data", but as I note in that thread it's not only hard to specify but probably doesn't actually produce behavior anyone wants.
Can't happen.
This really depends on how you define "properties". For example, stringify followed by parse will change your prototypes if you the parse in a different global from the stringify... Should that count? Probably not.
Lots of those cases, yes.
The primary issue is that the behavior is NOT DEFINED.
These are not nearly enough; see the public-script-coord spec.
This is not an invariant of the object graph, when proxies are involved. |
On Wed, Nov 30, 2016 at 1:20 PM, Rick Byers ***@***.***> wrote:
The value must satisfy:
- has typeof of "number", "string", "boolean", or "object"
Only "object" is allowed, because the spec says "JSON-serializable object".
|
@RByers applies these rules recursively to the property values, so they better allow more than just "object" unless you want to encode everything in the shape of the object graph itself. ;) |
To be clear, the way I would spec this is that the spec algorithm just calls JSON.stringify on the "data". If that throws, propagate out the exception. Then hand around the string. |
Ah! I saw the post but somehow thought there weren't any replies yet (damn I hate the hypermail UI). Sorry for the noise.
I had avoided that proposal because I thought it would be a potential interop issue since it's impossible to test that the UA is actually discarding all the non-serializable properties. But that's probably a silly concern - easy to satisfy via code inspection. So I agree - that's a much better proposal than what I was suggesting. |
We chatted about this on the editor call this morning. We'd be happy to merge in a PR that updated the spec to use json.stringify and propagate the exception. Could either @bzbarsky or @marcoscaceres take a stab at a PR for this? |
Are we sure we want to take an object as input, not an existing JSON string? If we are (e.g. if callers are expected to construct the object on the fly instead of producing it from JSON to start with), it would be good to sort out #346 and #338 too so the processing model is actually defined. It's hard to write a useful PR without knowing what should happen to the string after it's generated. |
1 similar comment
Are we sure we want to take an object as input, not an existing JSON string? If we are (e.g. if callers are expected to construct the object on the fly instead of producing it from JSON to start with), it would be good to sort out #346 and #338 too so the processing model is actually defined. It's hard to write a useful PR without knowing what should happen to the string after it's generated. |
To the extent that you use I agree with @bzbarsky that ECMAScript doesn't really define the machinery to detect without side effects whether an object is JSON-stringifyable as defined above. It is potentially possible to define it, but it would be pretty ugly (violating some invariants that we try to maintain about the JS object model) and a lot of work to get right. Given that users already have to worry about losing data when calling JSON.stringify directly, how much worse is it that they may also encounter that issue in this case? Is this part of the API already shipped to users in some browsers? If so, compatibility would be a pretty strong reason to continue taking an object as input, to be serialized to JSON, rather than a string, even if it's not conceptually the cleanest model. |
Shipped in Chrome since September 2016. |
And the thing that's shipped totally doesn't match what the spec is saying right now, which is the best part of the whole exercise, of course. |
I'd like to think that the implementation follows the spirit of the spec. The spec language should be improved, of course. If the implementation is doing something horrible, I'm happy to change it. |
The whole point of specs is to follow their letter, not just their spirit. Otherwise, why bother with a spec? And implementors who discover they can't follow the letter for some reason should raise corresponding spec issues. Which means this issue should have been raised before Google shipped. :( |
While the situation is not optimal, it seems like we have a concrete way to move forward here: continue with the reviews that Mozilla has kicked off, tighten up language and layering, and make web-compatible tweaks where it makes sense. This API gives a lot of value for users, and the Web has endured much worse, so I think we can get through this. Has anyone from the Chrome side documented the way that the implementation follows the "spirit" of the spec and any potential mismatches? If not, that would probably be a good exercise to complement what Mozilla has been contributing in bugs like this one. |
We run the object through JSON.stringify() and propagate any exceptions. |
I looked into fixing this and it's basically blocked on #346 and #338. That is, once I stringify the objects, what do we do with them? Just throw them away? Apparently they're passed on to the payment processor somehow, but that part of the spec is missing. I talked with @zkoch about what it should say roughly, so I can try a stab at it soon. |
After working on w3c/payment-method-basic-card#20 this is more confusing than ever. As far as I can tell the desired semantics for basic card are:
To me it seems like step 1 is entirely redundant, for basic card. If the desire is to support HTTP-based payment methods that require serialization, then they should define steps to serialize to JSON and send over HTTP and use that to compose a response. But for basic card, this shouldn't be necessary at all, since the normalization is already done by the dictionary conversion step. So JSON-serializability should be a payment-method-specific concern, not part of the overall PaymentRequest infrastructure. |
@domenic, you probably know best about the appropriate places to specify serializability. Here's my understanding of why the spec was written like this: Re: step 1, user agent should only JSON-stringify the data in order to pass it to the payment app. Here JSON-stringify is used as serialization for IPC. Payment apps are expected to be running in a different process, so all data destined for the payment app should be serializable. Re: step 2, JSON-parse and "convert data to a dictionary" are equivalent, no? This should happen in the payment app process, which has received the data serialized as a string. |
I think I see. Hmm, so if it's in another process, we can't do synchronous validation of the data, right? So we can't throw an exception in the constructor. In that case the flow should be something more like:
Does this sound right? |
This is right.
I think we get this for free. If the payment app is unable to handle a payment request, then it will be filtered out of possible payment methods. If no payment app is available to handle the payment request, then |
We should definitely write something down normatively. I don't quite understand the envisioned flow here though. When would this filtering happen? How does it know it's unable to handle the payment request, until it parses and converts the object in the other process? |
Stated another way: I had assumed steps 8/9 of https://w3c.github.io/browser-payment-api/#show-method were about very course-grained "this user agent accepts basic-card or not". But are you saying there is also some kind of implicit fine-grained introspection of the data there too? |
The filtering happens in the User agent checks the intersection of all the payment methods that the merchant website specified in
User agent cannot know this synchronously. The object needs to be serialized and passed to the payment apps first. @adamroach's latest take on the Payment App API has both enabledMethods sequence and canHandle(supportedMethods, shoppingCart) method that the payment app provides to the user agent at registration time. User agent can use these to asynchronously filter the payment apps that the user can select.
This depends on the payment app. Some payment app might tell user agent that it supports "basic-cad", period. That's coarse grained. An other payment app might inspect the |
OK, thanks so much! I think I see how this can work then. I'll try a new couple of PRs and you can tell me what you think of the result. |
This fixes w3c#338 and helps with w3c#307 by stating more explicitly how the data member of PaymentMethodDetails is serialized to a string, stored, and later passed to the appropriate payment app. Along the way, this fixes w3c#354. This does not completely fix w3c#307, as the problematic JSON-serializable concept is still used for PaymentDetailsModifier (and is still not used by any part of the processing model; that's w3c#346).
Note that one of the ways this could be a compat problem in practice is that (although it's obscure) some developers / libraries know to use a pattern like this to feature-detect support for dictionary members. From a quick skim I think this could be used today on Chrome to detect, for example, when some fields are being parsed and when they're not. If sites depended on that, it could become a compat problem to change it. |
This fixes w3c#338 and helps with w3c#307 by stating more explicitly how the data member of PaymentMethodDetails is serialized to a string, stored, and later passed to the appropriate payment app. Along the way, this fixes w3c#354. This does not completely fix w3c#307, as the problematic JSON-serializable concept is still used for PaymentDetailsModifier (and is still not used by any part of the processing model; that's w3c#346).
This fixes w3c#338 and helps with w3c#307 by stating more explicitly how the data member of PaymentMethodDetails is serialized to a string, stored, and later passed to the appropriate payment app. Along the way, this fixes w3c#354. This does not completely fix w3c#307, as the problematic JSON-serializable concept is still used for PaymentDetailsModifier (and is still not used by any part of the processing model; that's w3c#346).
This fixes #338 and helps with #307 by stating more explicitly how the data member of PaymentMethodDetails is serialized to a string, stored, and later passed to the appropriate payment app. Along the way, this fixes #354. This does not completely fix #307, as the problematic JSON-serializable concept is still used for PaymentDetailsModifier (and is still not used by any part of the processing model; that's #346).
Oh, oops, this should not have been auto-closed as the term is still used for PaymentDetailsModifier. I accidentally said "does not completely fix #307" which triggered GitHub's heuristics. If someone could reopen that would be great :). |
Sorry, my fault! :) |
* Editorial: rename "parsedMethodData" to "serializedMethodData" We actually store the serialized form, not the parsed form; this is more accurate. * State how payment details modifier data is serialized and used This closes #346, by making it clear that the data is serialized and stored in the PaymentRequest for further usage by show(), and closes #307, by finally stating all the points at which JSON-serialization happens explicitly. * Fixed [[\serializedModifierData]] nit
I might be wrong, but I recall "JSON-serializable object" coming up in the past in other specs and being problematic:
But I'm pretty sure it's a bit of a 🦄 - or it should be actually defined as to how one actually works that out. Right now, the spec basically implies:
That seems nasty. Could we check with public-script-coord what the right thing to do here is?
There might be an actual object type that we can use here.
The text was updated successfully, but these errors were encountered: