Generated schema contains errors

[sonntag-philipp]

Issues I noticed

• Use of anyOf should be replaced with oneOf after comparing the declarations and the resulting schema
• CustomElementDeclaration seems to only consist of ClassDeclaration while CustomElement is omitted completly.
  • This may apply to other properties as well, I didn't took a closer look.

I don't know how severe those issues are, but I would assume that they're pretty severe.

This an excerpt from the schema.json inside the package. It contains the definition for JavaScriptModule.

"JavaScriptModule": {
    "properties": {
        "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"
                    },
                    {
                        "$ref": "#/definitions/FunctionDeclaration"
                    },
                    {
                        "$ref": "#/definitions/MixinDeclaration"
                    },
                    {
                        "$ref": "#/definitions/VariableDeclaration"
                    },
                    {
                        "$ref": "#/definitions/CustomElementDeclaration"
                    }
                ]
            },
            "type": "array"
        },
        "description": {
            "description": "A markdown description of the module.",
            "type": "string"
        },
        "exports": {
            "description": "The exports of a module. This includes JavaScript exports and\ncustom element definitions.",
            "items": {
                "anyOf": [
                    {
                        "$ref": "#/definitions/JavaScriptExport"
                    },
                    {
                        "$ref": "#/definitions/CustomElementExport"
                    }
                ]
            },
            "type": "array"
        },
        "kind": {
            "enum": [
                "javascript-module"
            ],
            "type": "string"
        },
        "path": {
            "type": "string"
        },
        "summary": {
            "description": "A markdown summary suitable for display in a listing.",
            "type": "string"
        }
    },
    "required": [
        "kind",
        "path"
    ],
    "type": "object"
},

This is the CustomElementDeclaration from the same json

"CustomElementDeclaration": {
    "description": "A description of a custom element class.\n\nCustom elements are JavaScript classes, so this extends from\n`ClassDeclaration` and adds custom-element-specific features like\nattributes, events, and slots.\n\nNote that `tagName` in this interface is optional. Tag names are not\nneccessarily part of a custom element class, but belong to the definition\n(often called the \"registration\") or the `customElements.define()` call.\n\nBecause classes and tag anmes can only be registered once, there's a\none-to-one relationship between classes and tag names. For ease of use,\nwe allow the tag name here.\n\nSome packages define and register custom elements in separate modules. In\nthese cases one `Module` should contain the `CustomElement` without a\ntagName, and another `Module` should contain the\n`CustomElement`.",
    "properties": {
        "description": {
            "description": "A markdown description of the class.",
            "type": "string"
        },
        "kind": {
            "enum": [
                "class"
            ],
            "type": "string"
        },
        "members": {
            "items": {
                "anyOf": [
                    {
                        "$ref": "#/definitions/ClassField"
                    },
                    {
                        "$ref": "#/definitions/ClassMethod"
                    }
                ]
            },
            "type": "array"
        },
        "mixins": {
            "description": "Any class mixins applied in the extends clause of this class.\n\nIf mixins are applied in the class definition, then the true superclass\nof this class is the result of applying mixins in order to the superclass.\n\nMixins must be listed in order of their application to the superclass or\nprevious mixin application. This means that the innermost mixin is listed\nfirst. This may read backwards from the common order in JavaScript, but\nmatches the order of language used to describe mixin application, like\n\"S with A, B\".",
            "items": {
                "$ref": "#/definitions/Reference"
            },
            "type": "array"
        },
        "name": {
            "type": "string"
        },
        "source": {
            "$ref": "#/definitions/SourceReference"
        },
        "summary": {
            "description": "A markdown summary suitable for display in a listing.",
            "type": "string"
        },
        "superclass": {
            "$ref": "#/definitions/Reference",
            "description": "The superclass of this class.\n\nIf this class is defined with mixin\napplications, the prototype chain includes the mixin applications\nand the true superclass is computed from them."
        }
    },
    "required": [
        "kind",
        "name"
    ],
    "type": "object"
}

[justinfagnani]

Thanks for catching this @sonntag-philipp!

@thepassle @rictic we had removed schema.json from the repo because it's a generated file, but I wonder if we should add it back so that it can be reviewed?

[justinfagnani]

Looking at the schema a bit more carefully:

anyOf

I think anyOf might be ok for declarations, even if oneOf might be technically more correct.

Declaration is almost a discriminated union, in that there among ClassDeclaration, FunctionDeclaration, MixinDeclaration, VariableDeclaration the kind property is the discriminant. There is no way for an object to satisfy more than one of those interfaces at a time. CustomElementDeclaration and CustomElementMixinDeclaration don't have their own kinds because they are a ClassDeclaration and MixinDeclaration respectively with CustomElement schema mixed in. So an object would satisfy ClassDeclaration if it satisfied CustomElementDeclaration.

So it seems like there is no way for an object to incorrectly be valid for more than one of the subschemas. I did file #101 to see if we can get a way to generate oneOf however.

CustomElementDeclaration not including CustomElement

This is definitely bad in the 1.0.0 schema.

I just regenerated the schema with [email protected] and the structure is different. I think may be because I changed CustomElementDeclaration to an intersection type rather than an interface. At head, there is no CustomElementDeclaration at all, but in JavaScriptModule's declarations there's an allOf for ClassDeclaration and CustomElement`. This will perform the correct validation, though it's not as clear for documentation.

I'll look getting a more clear output and checking in the schema so we can review and diff.

[sonntag-philipp]

Thanks for the fast reply. I just took a look into the PR and added some feedback 👍

Hopefully the tests will prevent issues like those in the future :)