-
Notifications
You must be signed in to change notification settings - Fork 228
Add incremental schema loading to imodel schemacontexts #8594
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
base: master
Are you sure you want to change the base?
Add incremental schema loading to imodel schemacontexts #8594
Conversation
|
Can you please explain in the PR or issue description why we are making this change?
Why is it internal? Who is expected to call it? AFAICT, right now it is only used by tests. |
We added incremental schema loading as additional way to load schemas incrementally instead of all at once as JSON documents. After some testing now, we want to add the incremental schema locaters as default locaters on the schemacontexts of all IModels and IModelConnections replacing the default schemaProps loader. More information: Loading schemas incrementally via ECSql
It's internal because we don't want to get it added to the public API, only internal components (within itwinjs) shall enable it if they want to use it now. One candidate for that is presentation (who also requested the change in the first place). I can imagine after some additional test runs of several components, we might switch the default from disabled to enabled. |
|
There as got to be some better way to do this. Outside the scope of this one case we are going to need the ability to have some configuration to flag on/off code. Should each case be handled individually or should we have some pattern(s) to follow? |
|
|
||
| // Global options for incremental schema loading. | ||
| const schemaLoadingOptions = { | ||
| isEnabled: false, |
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.
from the name of the top level obj and this property, i would not have guessed this was wrt to incremental schema loading.
i dont think i love this approach of a psuedo global var that is checked in both frontend and backend. is there a reason why both frontend and backend need to respect this, and both of them (frontend and backend) could decide indepently which approach (incremental vs not) to use?
Our components that would benefit from incremental schema loading are outside of this repo, so any API they need to call should be public. However, why not just have it always enabled? I don't really like the idea of a single component deciding to change global state that affects the whole application. IMO, if for some reason we can't have that enabled by default, then it should be a configuration option at the application level. |
|
I'm currently trying to switch the incremental schema activation from the global function to the proposed application level setting. This is definitely a better approach as it would not require to rebuild the code if someone needs to change the setting. I saw there is an class Also, we follow Grigas comment and always activate the incremental schema loading by default, if a client runs into issues on their side, it can then use the app config to opt-out to get the previous behavior. |
Typically, apps configure the behavior of the core packages through IModelAppOptions (frontend) and/or IModelHostOptions (backend). The app can decide how to choose the configuration (e.g., using a feature flagging service, hard-coding features they always/never want enabled, whatever). The |
But that would again a code only approach, which means, clients have to specify their options explicitly on startup. On the other hand, if I look at the options in the IModelAppOptions or IModelHostOptions, the public settings rather allow to set whole components than having flags that control the behavior of certain features inside the internals IModels have to work as they work, which is why I feel currently hesitant to expose that as a public option. |
I'm afraid I don't follow your misgivings. For example, TileAdmin configuration provided at startup is exactly a set of "flags that control the behavior of certain features inside the internals", provided for seemingly the same purposes as your new feature (either opting in to a new experimental feature, or opting out of such a feature if it causes problems). |
Adds the incremental schema locaters to IModelDb and IModelConnection schemacontext as default locater.
Also introduces an internal api to enable/disable incremental schema loading through a helper method.
Default is currently set to
false.