Derived class caching #7429
Derived class caching #7429
Conversation
christophermlawson
requested review from
a team,
ColinKerr and
StefanApfel-Bentley
as code owners
| @@ -115,7 +115,7 @@ export class RelationshipClasses extends ECClasses { | |||
| try { | |||
| await this.validate(relClass!); | |||
| } catch(e: any) { | |||
| relClass!.baseClass = baseClass; | |||
| await relClass!.setBaseClass(baseClass); | |||
There was a problem hiding this comment.
Should we add some check here (or actually in line 111) to ensure that relClass is not undefined? Since it's a lookup by key, I assume it could be happen (eg when loading from a referenced schema) or did I missed some previous validation here?
There was a problem hiding this comment.
I added a catch block on line 112.
| /** | ||
| * Sets the base class of the ECClass. Pass undefined to 'remove' the base class. | ||
| */ | ||
| public async setBaseClass(baseClass: LazyLoadedECClass | undefined) { |
There was a problem hiding this comment.
Do we have to use a LazyLoaded class here? In all calls I saw, we've always had the baseClass resolved already, so there wouldn't be a need to do the detour via the delayed loaded proxies. Even when loading from JSON we expect the baseClass to be in the schema already (lookupItemSync in line 458).
That makes we wonder if we either missed a case where the baseClass is not yet present in the schema (aka lookup would fail) or if having the base class lazy-loaded is over-complicating things there as we have the items resolved already.
There was a problem hiding this comment.
I think this should be internal and protected. Let me think on this. The editing API should call this. The old property setter took the LazyLoaded object, and I don't think we should change it.
There was a problem hiding this comment.
FYI, I updated the method to protected. I know we discussed this on a call a while ago and decided to keep the LazyLoaded stuff for now.
| const baseClass = relClass?.baseClass; | ||
|
|
||
| await super.setBaseClass(itemKey, baseClassKey); | ||
|
|
||
| try { | ||
| await this.validate(relClass!); | ||
| } catch(e: any) { | ||
| await relClass!.setBaseClass(baseClass); | ||
| await (relClass! as ECClass as MutableClass).setBaseClass(baseClass); |
There was a problem hiding this comment.
why is a double cast necessary? especially given you already have the definitely assigned operator, which together makes this about the least type safe typescript way to accomplish what you seem to want to do here
There was a problem hiding this comment.
I know it looks odd, but it has to do with our Mutable classes, which do not derive from each other but from the class they 'mutating'. A RelationshipClass cannot be cast to a MutableClass directly, because they have no direct relationship. We have a MutableRelationshipClass but we can't use it because it does not derive from MutableClass, and we are trying to access members of MutableClass in this instance). We discussed this and there might be some TS trickery we can do to make the mutable classes more usable, but for now, this is what we have. The mutable classes are strictly internal and are not used outside the package.
Introduced derived class caching, which required a breaking change to the ECClass.baseClass property setter to the setBaseClass method.