w3c / web-nfc

Web NFC
https://w3c.github.io/web-nfc/
Other
313 stars 69 forks source link

Check 'typedef any NDEFRecordData' to be a JSON type? #307

Closed leonhsl closed 5 years ago

leonhsl commented 5 years ago

This question comes from webnfc impl in Chromium, to make clear how to impl step 1 of http://w3c.github.io/web-nfc/#mapping-json-to-ndef:

If the type of a record's data is not a JSON type, throw a TypeError and abort these steps.

while the record's data is defined as typedef any NDEFRecordData in the IDL file, I tried to look for help in blink-dev and got some quite helpful answer there.

With the following snippets, you can find more details on the thread https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/t-ehjfCrIgc.

The WebIDL spec does use the phrase "is a JSON type", but applies it to WebIDL values rather than to ECMAScript values, meaning the sort of WebIDL values that can legally be returned from toJSON. The NFC spec seems to be trying to discriminate on the type of the ECMAScript value, which isn't how this is supposed to work.

In general specs that test “JSONness” of a value are just broken.

We did a similar fix in Payment Request some years ago that you may find instructive. See https://github.com/w3c/payment-request/issues/307, https://github.com/w3c/payment-request/pull/382, and https://github.com/w3c/payment-request/pull/404

We may need to refine webnfc spec.

@riju @reillyeon @kenchris @domenic @marcoscaceres @beaufortfrancois

zolkis commented 5 years ago

If the type of a record's data is not a JSON type

Sloppy language. That algorithm is broken in multiple places, probably due to successive rewrites. For instance data is member of NDEFRecordInit only. I am pretty sure there would be formal concerns over the related algo, too: http://w3c.github.io/web-nfc/#creating-web-nfc-message

Perhaps we should use language from the Infra spec?

zolkis commented 5 years ago

Actually the algorithm works without that incriminated step 1 IMHO, it just builds an object from another and in that case it would ignore everything else from the source object that is not used for initializing an NDEF record. I don't see much added value in step 1, especially if it's so hard to express it in Web specs that it needs to involve serialize/deserialize algorithm just for validation in a use case that otherwise wouldn't need serialization.

@kenchris do you think we can just remove that step 1?

kenchris commented 5 years ago

while the record's data is defined as typedef any NDEFRecordData in the IDL file, I tried to look for help in blink-dev and got some quite helpful answer there.

We can actually just remove that and just use any

marcoscaceres commented 5 years ago

any is bad. Please don't use that. What are you trying to do, exactly?

kenchris commented 5 years ago

Basically almost anything can be considered JSON (numbers, objects, Uint8Array, false, etc etc, but not say undefined) and creating a specific type union for that is basically impossible - thus any makes a lot of sense here. If the recordType is "json" just tread the value as JSON.

kenchris commented 5 years ago

@zolkis I had no idea which step 1 you were referring to

If the type of a record's data is not a JSON type, throw a TypeError and abort these steps.

Yes, I think that is fine, we already throw earlier if it is undefined

kenchris commented 5 years ago

Domenic comments here makes sense

In general specs that test “JSONness” of a value are just broken. Instead what you want to do is serialize the data to a string (or bytes) and then pass around the serialization as appropriate. I.e. there should be explicit serialization and deserialization steps, which talk about how to handle cases where serialization fails (because, for example, someone passed in the object { get x() { throw new Error(); } }).

We are testing undefined earlier but people could supply a non JSON type like a Symbol. So we just need to make sure that we handle serialization errors and then throw

marcoscaceres commented 5 years ago

(I've not looked at what you are doing at all... but... :) )

Ok, if this is coming from a random source, then indeed any is your only option.

If there is going to be some structure, then it's possible to use a dictionary.

kenchris commented 5 years ago

There is not in this case, no

kenchris commented 5 years ago

Ok looked at ECMAScript spec, in cases of unserializable values you just get undefined back:

https://tc39.es/ecma262/#sec-json.stringify

image

So we can throw on that, or just store 0 bytes (which might make more sense).

Then question is what happens currently if we are reading a JSON MIME type with 0 bytes written? Do we parse that back as undefined or null or does it throw?

image

I guess with 0 bytes payload we can just return undefined - that kind of makes sense

marcoscaceres commented 5 years ago

Wait, you didn't answer my question... in which direction is the data flowing? Is the developer supplying this JSON or is some NFC source supplying the JSON?

kenchris commented 5 years ago

The user is saying that they want to save the record data as json and then supplying the data, which in JSON terms can actually be anything - it is not much different than calling JSON.stringify manually

Of course, when reading it will be the other way around

I find it really weird if we say that you can store as JSON, but then limit it to dictionaries as JSON doesn' t have that limitation

marcoscaceres commented 5 years ago

The user is saying that they want to save the record data as json

What is a "user" here tho? Naturally, it's not a human writing JSON. So who is "user"?

kenchris commented 5 years ago

I don't follow. The user is the developer. They just show the intent to store something as JSON, just like when they call JSON.stringify(any), like I might very well do JSON.stringify([1, 2, 3, "Kenneth"]) or simialr with the Web NFC API

marcoscaceres commented 5 years ago

Ok, I'm a bow out of this discussion as I'm not educated/involved in this API and it's not your responsibility to educate me :) Maybe we can chat about it in person at TPAC.

kenchris commented 5 years ago

Sure! But generally I agree with you that you could try to use structured data in APIs

kenchris commented 5 years ago

Filed https://github.com/whatwg/infra/issues/264