webcomponents / webcomponents/custom-elements-manifest
Sum type ambiguities lead to incorrect or unclear validations
- Dominant language
- TypeScript
- Stars
- 502
- Forks
- 27
- PR merge metrics
- No merged PRs in 30d
Description
The json schema contains many anyOfs, which cause failures in a subkind of a type to continue up to the next sibling kind. Concrete examples:
- `ClassDeclaration` validates with invalid `CustomElementDeclaration` members
- an invalid `ClassDeclaration` will be validated against `FunctionDeclaration` in a `Module`'s `declarations` field.
given
```json
{
"schemaVersion": "2.1.0",
"modules": [
{
"kind": "javascript-module",
"path": "invalid-attribute.js",
"declarations": [
{
"kind": "class",
"name": "InvalidAttribute",
"tagName": "invalid-attribute",
"customElement": true,
"attributes": [
{
"summary": "nameless attribute"
}
]
}
]
}
]
}
```
```bash
ajv --strict=false \
-s ~/.cache/cem/schemas/2.1.0.json \
-d cmd/fixture/invalid-attribute/custom-elements.json
```
**Expected**: fails because `Attribute` (only possible on a `CustomElementDeclaration`) must have a `name` field.
**Actual:**
```
cmd/fixture/invalid-attribute/custom-elements.json valid
```
The reason this validates is because declarations[0] is first tried as a custom element, but *because the attribute doesn't have a name*, the validator continues to try it as a class declaration, and that passes. So there are two problems here
1. a `"kind": "class"` can validate as a ClassDeclaration, even if it contains forbidden CustomElementDeclaration subkinds. This is the essential problem
2. a `"kind": "class"` that has `"customElement": true` is sometimes not validated as a CustomElementDeclaration. it should *always* be validated as such
Related to #134, but if this can be solved without changing the schema implementation, it should be. I'm sure this isn't the only such example.
The solution I found was to replace "anyOf" with "if/then" for our sum types. This patch should hopefully fix all such ambiguities. In my testing at least, it does the trick. If this proposal is accepted, I'll work to update the build tooling to prefer if/then to anyOf for typescript discriminated unions.
```patch
diff --git a/2.1.0.json b/2.2.0.json
index d9e45b7..4823a82 100644
--- a/2.1.0.json
+++ b/2.2.0.json
@@ -63,14 +63,34 @@
},
"members": {
"items": {
- "anyOf": [
- {
- "$ref": "#/definitions/ClassField"
+ "if": {
+ "properties": {
+ "kind": {
+ "const": "field"
+ }
},
- {
+ "required": [
+ "kind"
+ ]
+ },
+ "then": {
+ "$ref": "#/definitions/ClassField"
+ },
+ "else": {
+ "if": {
+ "properties": {
+ "kind": {
+ "const": "method"
+ }
+ },
+ "required": [
+ "kind"
+ ]
+ },
+ "then": {
"$ref": "#/definitions/ClassMethod"
}
- ]
+ }
},
"type": "array"
},
@@ -750,26 +770,106 @@
"declarations": {
"description": "The declarations of a module.\n\nFor documentation purposes, all declarations that are reachable from\nexports should be described here. Ie, functions and objects that may be\nproperties of exported objects, or passed as arguments to functions.",
"items": {
- "anyOf": [
- {
- "$ref": "#/definitions/ClassDeclaration"
+ "if": {
+ "properties": {
+ "customElement": {
+ "const": true
+ }
},
- {
- "$ref": "#/definitions/FunctionDeclaration"
+ "required": [
+ "customElement"
+ ]
+ },
+ "then": {
+ "if": {
+ "properties": {
+ "kind": {
+ "const": "class"
+ }
+ },
+ "required": [
+ "kind"
+ ]
},
- {
- "$ref": "#/definitions/MixinDeclaration"
+ "then": {
+ "$ref": "#/definitions/CustomElementDeclaration"
},
- {
- "$ref": "#/definitions/VariableDeclaration"
+ "else": {
+ "if": {
+ "properties": {
+ "kind": {
+ "const": "mixin"
+ }
+ },
+ "required": [
+ "kind"
+ ]
+ },
+ "then": {
+ "$ref": "#/definitions/CustomElementMixinDeclaration"
+ }
+ }
+ },
+ "else": {
+ "if": {
+ "properties": {
+ "kind": {
+ "const": "class"
+ }
+ },
+ "required": [
+ "kind"
+ ]
},
- {
- "$ref": "#/definitions/CustomElementDeclaration"
+ "then": {
+ "$ref": "#/definitions/ClassDeclaration"
},
- {
- "$ref": "#/definitions/CustomElementMixinDeclaration"
+ "else": {
+ "if": {
+ "properties": {
+ "kind": {
+ "const": "function"
+ }
+ },
+ "required": [
+ "kind"
+ ]
+ },
+ "then": {
+ "$ref": "#/definitions/FunctionDeclaration"
+ },
+ "else": {
+ "if": {
+ "properties": {
+ "kind": {
+ "const": "mixin"
+ }
+ },
+ "required": [
+ "kind"
+ ]
+ },
+ "then": {
+ "$ref": "#/definitions/MixinDeclaration"
+ },
+ "else": {
+ "if": {
+ "properties": {
+ "kind": {
+ "const": "variable"
+ }
+ },
+ "required": [
+ "kind"
+ ]
+ },
+ "then": {
+ "$ref": "#/definitions/VariableDeclaration"
+ }
+ }
+ }
}
- ]
+ }
},
"type": "array"
},
@@ -787,14 +887,34 @@
"exports": {
"description": "The exports of a module. This includes JavaScript exports and\ncustom element definitions.",
"items": {
- "anyOf": [
- {
- "$ref": "#/definitions/JavaScriptExport"
+ "if": {
+ "properties": {
+ "kind": {
+ "const": "js"
+ }
},
- {
+ "required": [
+ "kind"
+ ]
+ },
+ "then": {
+ "$ref": "#/definitions/JavaScriptExport"
+ },
+ "else": {
+ "if": {
+ "properties": {
+ "kind": {
+ "const": "custom-element-definition"
+ }
+ },
+ "required": [
+ "kind"
+ ]
+ },
+ "then": {
"$ref": "#/definitions/CustomElementExport"
}
- ]
+ }
},
"type": "array"
},
```
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.