Skip to content
This repository has been archived by the owner on Jan 27, 2022. It is now read-only.

Type Guards for union type nodes report false negatives #135

Closed
alex-ketch opened this issue Aug 23, 2019 · 3 comments · Fixed by #138
Closed

Type Guards for union type nodes report false negatives #135

alex-ketch opened this issue Aug 23, 2019 · 3 comments · Fixed by #138
Assignees
Labels
bug Something isn't working released

Comments

@alex-ketch
Copy link
Contributor

Currently the isA and isType type guards only compare the passed type value, and don't take into account broadening of types, and will return false negatives.

isA('CreativeWork', {type: 'Article', ...}) // false
isA('MediaObject', {type: 'ImageObject', ...}) // false

isType('CreativeWork')({type: 'Article', ...}) // false
isType('MediaObject')({type: 'ImageObject', ...}) // false

These become an issue when used as filters in Encoda, mistakenly discarding nodes as being invalid.

Possible solutions:

  1. Create a mapping between types that are supersets of others, and use it to find the necessary typeMap and use it to test the node in question. Concern here being that I'm not 100% how to provide an exhaustive type signature just for these nodes, so it will have to be maintained manually.
  2. Create more typeMap filter variants, and deprecate the usage of isA and isType functions. Drawback being more changes to Encoda, and clunkier ergonomics for defining ad-hoc type .guards
@alex-ketch alex-ketch added the bug Something isn't working label Aug 23, 2019
@nokome
Copy link
Member

nokome commented Aug 23, 2019

Yes, I agree this is an issue. I suggest that we go down the route of (1). We already have the "mapping" in the form of each type's type e.g.

export interface MediaObject extends CreativeWork {
    type: 'MediaObject' | 'AudioObject' | 'ImageObject' | 'VideoObject';

but unfortunately those are not available at runtime. But, if we generated enums instead we would get the lookup table at runtime to be able to be used in type guards:

enum MediaObjectTypes {
   MediaObject = 'MediaObject',
   AudioObject = 'AudioObject',
   ImageObject = 'ImageObject',
   VideoObject = 'VideoObject'
}
export interface MediaObject extends CreativeWork {
    type: MediaObjectTypes

@alex-ketch alex-ketch self-assigned this Aug 26, 2019
@nokome
Copy link
Member

nokome commented Aug 27, 2019

Having though about this some more, I think we should address this at the JSON Schema level, with derived typings in each language. In the schema, we also need to be able to say things like "a CreativeWork, or anything that extends it". For, that we created CreativeWorkTypes.schema.yaml which is a manually authored (and thus potentially wrong) list of all types derived from CreativeWork.

Instead of manually authoring files such as CreativeWorkTypes.schema.yaml and manually writing files such as ts/util/type-maps.ts, I suggest that:

  • In ts/schema.ts we generate CreativeWorkTypes.schema.json, MediaObjectTypes.schema.json etc based on the descendants property.
  • In ts/bindings/typescript.ts we generate type-maps.ts from those *.schema.json files
  • In guards.ts we add a isDerivedFrom or isInstance or doesExtend that checks against those type maps (that would keep isA, for cases where you want to check that the node was of a particular type).

stencila-ci added a commit that referenced this issue Jan 19, 2020
# [0.33.0](v0.32.4...v0.33.0) (2020-01-19)

### Features

* **JSON Schema:** Generate union type for descendant types ([3376d73](3376d73))
* **Type Guards:** Add isInstanceOf guard for matching descendant types ([9985936](9985936)), closes [#135](#135)
@stencila-ci
Copy link
Collaborator

🎉 This issue has been resolved in version 0.33.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants