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

Meta: export convert an Infra value to a (JSON) JavaScript value #642

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

annevk
Copy link
Member

@annevk annevk commented Aug 26, 2024

Also known as convert an Infra value to a JSON-compatible JavaScript value, but that exceeded 72 characters.


I use this in w3c/push-api#385.


Preview | Diff

Also known as convert an Infra value to a JSON-compatible JavaScript value, but that exceeded 72 characters.
@annevk annevk requested a review from domenic August 26, 2024 15:19
@domenic
Copy link
Member

domenic commented Aug 26, 2024

I tried to figure out how you're using this, and it seems to not have much to do with JSON. Can't Web IDL handle the conversion from Infra types directly? We already said, e.g., that maps and dictionaries are identified, and that sequences and lists are identified.

@annevk
Copy link
Member Author

annevk commented Aug 27, 2024

I'm starting from a JSON document. A subset of that JSON document needs to be IDL-validated as per NotificationOptions to ensure things are strings, booleans, etc. The remainder can be dealt with at the Infra level.

So https://pr-preview.s3.amazonaws.com/w3c/push-api/pull/385.html#dfn-declarative-push-message-parser goes to Infra values first and then for the subset that needs to be IDL-validated it goes back to JS. It needs to go back because otherwise you cannot apply IDL validation. If I would just treat it as a dictionary I would actually bypass the validation that IDL provides.

@domenic
Copy link
Member

domenic commented Aug 28, 2024

Hmm. You could almost just say "If notificationInput is not a NotificationOptions dictionary", but I guess you want to allow extra keys and then filter them out as part of the conversion process. And apply default values from the dictionary definition. Still, it feels pretty strange to go through JS and then back to IDL/Infra.

Would you consider instead adding something to Web IDL which converts an Infra value into a dictionary type by removing all the excess keys / applying defaults? I guess that would also have to deal with required dictionary members and throwing on them... which is not fun.

OK, I'm basically sold. But a few things to consider:

  • Should we expose this low-level internal operation from Infra, or should we instead try to add a better wrapper for it (probably to Web IDL?) which converts Infra values to IDL types generally? We'd still end up needing to export this from Infra, but we could at least warn that it's not generally meant to be used and the IDL wrapper is better.

  • Are you sure you really want the Web IDL conversion behavior here? For example, if someone passes the JSON { "lang": null, "renotify": 0, "actions": [{ "action": false, "title": 5 }] this will be converted to { lang: "null", renotify: false, actions: [{ action: "false", title: "5" }]}. This is a bit unusual, I think... have you checked what other JSON-accepting parts of the web platform do with wrongly-typed values? E.g. import maps drops map entries which use the wrong types.

@annevk
Copy link
Member Author

annevk commented Aug 28, 2024

I'm not concerned about the map containing additional keys per se. It's not like "create a notification" would do anything with unknown keys.

I am concerned about type coercion and defaulting. I don't want "create a notification" to be invoked with a map that it did not expect, where title is a number or some such.

I think you are correct that other JSON formats might not do type coercion so perhaps I should duplicate the dictionary semantics, modulo some of the type coercion (we'd still do string -> scalar value string though). (Ideally someone solves #159.)

However, data needs to be a JS value so for that I'd still need this algorithm even if we decide to go down that route.

@annevk
Copy link
Member Author

annevk commented Aug 28, 2024

Doing validation by hand does raise some questions that will likely be answered differently across JSON consumers:

  • Is an incorrect type a failure or cause to ignore the field? (Failure means it'll be harder to extend going forward so I would be inclined to ignore, but I see that failure is used in certain existing cases.)
  • Is a lone surrogate an incorrect type or should it be U+FFFD'd? (I would be inclined to U+FFFD.)

@domenic
Copy link
Member

domenic commented Aug 28, 2024

I would support efforts to make this more uniform, maybe with Infra...

I think U+FFFD is generally good, but skipping vs. erroring might need to be format-specific depending on extensibility requirements.

@annevk
Copy link
Member Author

annevk commented Aug 28, 2024

Yeah same, but I've already done a lot of yak shaving on this particular feature. If you have thoughts on what uniform behavior should be I'm happy to take that into account of course. It sounds like you'd prefer Infra value type checking over IDL dictionary conversion which seems like a reasonable enough change, even though it introduces a fair bit of redundancy.

@domenic
Copy link
Member

domenic commented Aug 28, 2024

Yeah, I think IDL is just not the right tool for JSON usually, thus the discussions in #159.

From what I recall of the formats I've had a part in:

  • Unexpected types are always "rejected", never coerced.
  • What "rejected" means varies widely: sometimes we ignore the value (if it's an optional one, or e.g. an array entry). Sometimes we throw out some larger surrounding value. Sometimes we throw out the entire JSON.
  • Unexpected keys are usually ignored but in some special cases it's better for them to cause rejections (of some scope) since we assume a future key would change the meaning substantially.

Anyway I guess I should approve this because you're right that you'll need it for data. It still feels a bit open to abuse but I can't think of any better alternative.

@annevk annevk merged commit 4db814c into main Nov 6, 2024
2 checks passed
@annevk annevk deleted the annevk/infra-to-js branch November 6, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants