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

Don't use enums, because they throw #633

Closed
marcoscaceres opened this issue Dec 21, 2017 · 10 comments
Closed

Don't use enums, because they throw #633

marcoscaceres opened this issue Dec 21, 2017 · 10 comments

Comments

@marcoscaceres
Copy link
Member

We need to drop all the enums, as they throw.

@RobDolin
Copy link

Are you thinking they need to be switched to strings with sets of known values defined in the spec?

@marcoscaceres
Copy link
Member Author

yes, exactly.

@kenchris
Copy link
Collaborator

kenchris commented Feb 22, 2018

that won't work for the enums we get from other specs like Orientation Lock and Service Workers, so we better find a way to handle the throws

@marcoscaceres
Copy link
Member Author

I don't have a good solution for this. I wouldn't really be in favor of modifying WebIDL to not throw for enums, so we might just have to maintain our own copy of supported values.

@lodott
Copy link

lodott commented Aug 23, 2018

I have been playing around with manifest parsing via WebIDL spec recently and may have some options that are hopefully useful to you.

First off, for WebAppManifest as currently defined, I found the following situations which can potentially throw TypeErrors:

  • DOMString/USVString, if a Symbol is assigned
  • Enums, if an invalid value is assigned
  • Dictionary members marked required, if omitted

The first can be ignored for WebAppManifests as the values come from parsing JSON which should not be able to produce Symbols.

The other two can arise and formally would invalidate the WebAppManifest, if WebIDL conversion is followed strictly.

My suggestion:

  • by default require strict WebIDL conversion, so improperly written manifests are indeed invalidated - since improper JSON is rejected, why not simply extend this behavior to improper values in a proper JSON?
  • that said, via MAY/SHOULD language user-agents should be allowed/encouraged to use their own custom variants of WebIDL conversion, that handle TypeErrors as they see fit - this covers how user-agents currently behave anyway
  • the TypeErrors that are allowed to be handled in a custom fashion can optionally be restricted to the relevant cases mentioned above

There is also relevant discussion going in #710 currently, which probably should better be continued here?

@marcoscaceres
Copy link
Member Author

by default require strict WebIDL conversion, so improperly written manifests are indeed invalidated - since improper JSON is rejected, why not simply extend this behavior to improper values in a proper JSON?

The problem is that we already have thousands of manifests in the wild that are depending on the fault-tolerant behavior.

that said, via MAY/SHOULD language user-agents should be allowed/encouraged to use their own custom variants of WebIDL conversion, that handle TypeErrors as they see fit - this covers how user-agents currently behave anyway

I'm not sure that is true. In Gecko, we don't do any IDL conversion. Additionally, It's too much work to have a custom IDL implementation for this or other formats. It's just not a viable thing.

the TypeErrors that are allowed to be handled in a custom fashion can optionally be restricted to the relevant cases mentioned above

This doesn't sounds like it would lead to very interoperable behavior.

There is also relevant discussion going in #710 currently, which probably should better be continued here?

Probably :)

@mgiuca
Copy link
Collaborator

mgiuca commented Aug 23, 2018

Yeah I don't think letting error handling be up to user agents is a good idea.

What I said on #710:

We can write a top-level rule that says "if the ECMAScript to IDL conversion throws a TypeError, the manifest is invalid", so it isn't meaningless that the conversion throws a TypeError outside of an actual JavaScript execution context. BUT the problem is that we don't want to trash the whole manifest if one member is invalid. If one member throws a TypeError, we generally want to ignore that member, not the whole thing.

It seems that this isn't an issue with using IDL in a document format, but rather that IDL's conversion algorithm doesn't have a "non-strict mode" (that I know of). If there was a way to say "convert an ECMAScript object to IDL, with any TypeErrors in a dictionary member causing that member to be set as undefined". Or thereabouts. That would be perfectly usable in this spec.

@marcoscaceres response:

That's really not feasible. We don't need to fork our IDL biding layer for this format.

I think we need to go and figure out whatwg/infra#159

I agree if we can avoid using JavaScript conversion at all for these JSON documents, that would be good. But I do think we want to declare a complex JSON document format in a declarative language, not as an algorithm, and WebIDL seems the most appropriate.

We could also use JSON Schema which is specifically designed for this use case (but I'm not familiar with the details, and a quick scan of the documents doesn't seem to let us process a JSON object being lenient about invalid values; it's either valid or not).

@lodott
Copy link

lodott commented Aug 23, 2018

We could also use JSON Schema which is specifically designed for this use case (but I'm not familiar with the details, and a quick scan of the documents doesn't seem to let us process a JSON object being lenient about invalid values; it's either valid or not).

The JSON Schema allows for conditional subschemas which should make it possible to assign alternatives for cases where WebIDL throws.

Referencing WebIDL definitions for use in the JSON schema outside of the specification may require some thought this way, but this looks good to me on first impression.

@marcoscaceres
Copy link
Member Author

The JSON Schema allows for conditional subschemas which should make it possible to assign alternatives for cases where WebIDL throws.

We've tried schema languages in the past and they don't really work well on web scale - authors make too many mistakes, and we fall into a "the tools will save us" situation.

I also don't want to maintain a JSON Schema (or similar) implementation in Gecko :( ... I know, I'm super lazy.

Referencing WebIDL definitions for use in the JSON schema outside of the specification may require some thought this way, but this looks good to me on first impression.

What we had originally wasn't bad - and quite fault tolerant. I think everyone is using a custom processor for Web Manifest, so that would most closely match reality.

@marcoscaceres
Copy link
Member Author

In 32b497c, I (finally) got rid of the IDL and converted the "processed manifest" to an (Infra) ordered map.

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

Successfully merging a pull request may close this issue.

7 participants