iTwin / itwinjs-core

Monorepo for iTwin.js Library
https://www.itwinjs.org
MIT License
615 stars 210 forks source link

Why does `ElementAspectProps` have `element` and `jsonProperties` properties? #3968

Open jackson-at-bentley opened 2 years ago

jackson-at-bentley commented 2 years ago

These properties are not defined in BIS, and aren't defined on the class type. Are they ignored by the iModel?

I'm finding the divergence between the class and object types kind of odd.


⚠ Do not edit this section. It is required for imodeljs.github.io ➟ GitHub issue linking

pmconne commented 2 years ago

The ElementAspect TypeScript class does have an "element" property. It's probably there for convenience because both of its direct subclasses - ElementUniqueAspect and ElementMultiAspect - have an "element" property (in BIS).

The ElementAspect TypeScript class has a "jsonProperties" property because it derives from Entity. This is probably a mistake - given it's not defined in the ECSchema, I don't see how it could get persisted. @diegoalexdiaz?

diegoalexdiaz commented 2 years ago

There are a number of properties that got introduced into subclasses but conceptually they are the same thing so they could've been introduced at a base class level. E.g. ElementAspect.Element, GeometricElement.Category/GeometryStream. The TypeScript APIs are therefore conceptually more correct in that regard.

I'll discuss at the BWG whether there is low-risk in introducing base-properties for those classes in the BisCore schema, while still keeping their current implementations in sub-classes since they introduce ForeignKeyConstraints.

Regarding the "JsonProperties" case, it looks like a mistake in Typescript. The only ElementAspect-subclass that such a property got added to is "ExternalSourceAspect" - but such property is not meant to be applicable to all ElementAspects in general.