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

Consistently handle optional properties in metadata implementations #122

Closed
javagl opened this issue May 1, 2024 · 0 comments · Fixed by #117
Closed

Consistently handle optional properties in metadata implementations #122

javagl opened this issue May 1, 2024 · 0 comments · Fixed by #117

Comments

@javagl
Copy link
Contributor

javagl commented May 1, 2024

The implementations of the glTF metadata extensions contain definitions of "model classes" that offer the actual structures and functionalities of the extensions. And they contain lower-level interface definitions that, roughly speaking, ensure type-safety in these classes. Some details are explained in this inline comment.

Right now, these interfaces do not properly reflect the optional and required properties. For example, they define some example: string property, but when this property is optional, then it should be example: string | null.

Until now, this was not a critical issue: The model objects are created, and when they don't receive a value for this property from the JSON input, then the value just remained undefined. But... the model classes derive the signature of their methods from these interfaces! And therefore, this can cause two more pressing issues:

  • The model class would offer a method like getExample() : string, asserting that the result should never be undefined - but it could be undefined if no value was given in the JSON input
  • Similarly, the model offers a method like setExample(example: string), which would not accept undefined as a parameter.

(The latter became apparent in a rabbit hole related to #117, where it was necessary to set the schemaUri of a StructuralMetadata to undefined, and this wasn't allowed...)

All properties should be checked accordingly, based on whether they are required or optional in the schema.

EDIT: An aside: Some details about null vs. undefined may require some attention here. From what I've seen, the glTF-Transform "model" classes generally never use undefined anywhere. They always use null. I'm not sure about the reasoning, but assume that the metadata extension implementations should follow whatever conventions already exist, and handle optional properties via null.

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

Successfully merging a pull request may close this issue.

1 participant