-
Notifications
You must be signed in to change notification settings - Fork 216
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
ecschema-metadata integration in backend #7577
base: master
Are you sure you want to change the base?
Conversation
3616a3f
to
e02f8b1
Compare
public getItemSync<T extends SchemaItem>(name: string): T | undefined { | ||
// Case-insensitive search | ||
return this._items.get(name.toUpperCase()) as T; | ||
public getItemSync<T extends SchemaItem>(name: string, itemConstructor?: SchemaItemConstructor<T>): T | undefined { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to take in just typeof SchemaItem
and have it just behave the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmh it doesn't quite work like that. I tried so many variations now....
public getItemSync<T extends SchemaItem>(name: string, itemConstructor?: typeof SchemaItem): T | undefined {
I think this is what you had in mind. The problem with that is, that itemConstructor is not connected to T in this case. So you could call getItemSync<EntityClass>("name", Unit);
which does not make any sense.
What DOES work is this:
public getItemSync<T extends typeof SchemaItem>(name: string, itemConstructor?: T): InstanceType<T> | undefined {
const value = this._items.get(name.toUpperCase());
if (value === undefined)
return value;
if (itemConstructor !== undefined && (value.schemaItemType !== itemConstructor.schemaItemType))
throw new ECObjectsError(ECObjectsStatus.InvalidSchemaItemType, `The item ${name} is not of the expected type.`);
return value as InstanceType<T>;
}
But that is a break, because now the generic parameter has to be typeof EntityClass
so everybody calling the method with getItemSync<EntityClass>
has to adjust it to getItemSync<typeof EntityClass>
. I guess we could do that, but I'm hesitant...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reworked it a bit. Strangely, when I put SchemaItemConstructor into SchemaItem.ts and import it, the compiler would complain it cannot find the schemaItemType static property on the constructor. But IntelliSense didn't complain.
When I moved the type into the Schema.ts file, everything suddenly worked fine...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is cleaner but I'm concerned that this method could be suprising to callers. If called without the constructor it will return undefined if no item is found, the item found cast to the template type regardless of the actual type. If called with the constructor it will return undefined if no item is found, the item if found and of the right type or throw if an item of the wrong type is found.
I propose that we provide two options:
- Explicit typesafe options that return the typed item if found and of the right type or undefined if not found or not of the right type
- Explicitly generic option that returns a SchemaItem if found and leaves it up to the user to do the type checking
I suggest this because those two methods seem to cleanly fit the usecases we've seen over the years. e.g. If I need to use a unit called smoot any other item called smoot won't do. If I want to insert a unit called smoot I want to know if any other item exists regardless of type.
What do you think?
…ma keys and metadata retrieval
…Schema type directly
…ng for schema item retrieval
…ity and consistency in metadata retrieval
…methods and improved type checking for better metadata management
…for improved type safety and clarity in metadata handling
…type safety and clarity
…improved type safety
88a8790
to
b816e07
Compare
Remove deprecated synchronous getter methods from Schema class
…n and pnpm-lock.yaml
@@ -92,6 +108,25 @@ export class Entity { | |||
/** Get the full BIS class name of this Entity in the form "schema:class". */ | |||
public get classFullName(): string { return this._ctor.classFullName; } | |||
|
|||
public get schemaItemKey(): SchemaItemKey { return this._ctor.schemaItemKey; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets tag everything we're adding as @beta during development so we can go through and review everything one last time before we make 5.0 final
@@ -1076,6 +1079,19 @@ export abstract class IModelDb extends IModel { | |||
return this._classMetaDataRegistry; | |||
} | |||
|
|||
public get schemaContext(): SchemaContext { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add documentation now so we don't have a debt of docs to fill out at the end. It will also help other people not familiar with ecschema-metadata understand the changes
/** Unique identifier for this schema, typed variant of [[schemaName]]. | ||
* @internal | ||
*/ | ||
public static get schemaKey(): SchemaKey { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this Schema object represents the version of the schema the domain handlers are written against you could make an argument that the version does matter.
I think what you have done is fine for now but our thinking here should be critical of the purpose of the Schema objects. That may guide us to change this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was really thinking of not going with SchemaKey/SchemaItemKey because backend as it stands does NOT care about version. And version may differ based on imodel. So if anything, we may hold a "minimum supported version" somewhere...
I have the impression people just want to identify their schemas by name and their items by fullname, which is fine in the context of a single imodel. So if we moved away from static stuff, we may want to rework this. I'd like to hear opinions on this from consumers.
@@ -19,6 +19,9 @@ export class ECVersion { | |||
private _write: number = 0; | |||
private _minor: number = 0; | |||
|
|||
// Define a static global instance representing zero | |||
public static readonly ZERO = new ECVersion(0, 0, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would be better to call this 'NoVersion' and document that a version with all zeros is an invalid version for a schema but using it in a key means no version has been specified. As it stands this could mean anything and the docs don't help you figure what it should mean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to keep track of the breaking changes we are making. For example removing generics from ISchemaLocater.
This list should exist outside the scope of this PR so we can write up a nice doc for the 5 release.
First step of cleaning up ClassRegistry, removing redundancy and make ecschema-metadata available in backend.
Story: https://github.com/iTwin/itwinjs-backlog/issues/1156
This is the first of several PRs, it only adds the dependency and adds some ways to access the data, it does not replace any existing logic yet.