search

iTwin / itwinjs-core

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

Evaluate the release tags as well as the stability of BackendHubAccess and hubAccess #4885

Closed nick4598 closed 1 year ago

nick4598 commented 1 year ago

BackendHubAccess: https://github.com/iTwin/itwinjs-core/blob/master/core/backend/src/BackendHubAccess.ts#L189 hubAccess: https://github.com/iTwin/itwinjs-core/blob/master/core/backend/src/IModelHost.ts#L110

The purpose of BackendHubAccess is to contain "Methods for accessing services of IModelHub from an iTwin.js backend".

hubAccess is a property within the IModelHostOptions interface and is of type BackendHubAccess. Both hubAccess and BackendHubAccess are marked as beta. These are both "critical to almost all workflows" and we should get to a place where these are stable from release to release.

Is there anything missing from this interface? Is there anything we shouldn't promote in this interface?

EDIT: Getting the iModelHub team's input/feedback will be important for this as well

aruniverse commented 1 year ago

fyi @austeja-bentley

austeja-bentley commented 1 year ago

As for additional functions - I think it is hard to predict what will be needed and I'd argue that this interface (as all other interfaces) should be as minimal as possible to cover the required workflows. Unless there are specific features planned for near future from iTwin.js side, I see no point in adding additional methods.

As for adding functionality in between major iTwin.js version releases maybe optional interface methods could be used, where iTwin.js could extend the BackendHubAccess interface with an optional method definition and support the new platform functionality only if the BackendHubAccess implementation has that method implemented. @nick4598 pointed out that they did consider this approach for one of the features. This sounds rather complicated/confusing from the user's perspective where a package upgrade could cause different code behavior, but I'd still consider it an option nonetheless.

Question - will the @internal and @deprecated methods be promoted to @public?

@Animagne would be interesting to hear your thoughts on this.

nick4598 commented 1 year ago

Internal and deprecated methods will not be promoted to public and will stay as they are. The logic being that all functions marked as internal will only be used by iTwin.js libraries and shouldn't need to be directly called by a user. @wgoehrig Anything to add to the internal vs public question? I know we were talking about this last week.

austeja-bentley commented 1 year ago

all functions marked as internal will only be used by iTwin.js libraries and shouldn't need to be directly called by a user.

aren't all functions in BackendHubAccess intended to be called only by iTwin.js? I had the impression that this interface was just a building block to complete the iTwin.js functionality. If user wants to do anything else with the iModels API (or any other API that stores iModels for that matter) they should use other libraries (@itwin/imodels-client-management in iModels API case). At least I got the impression that a lot of applications do this.

Animagne commented 1 year ago

I definitely think that something needs to be done with versioning these interfaces. Updating imodels-clients should be easier than updating entire iTwin.js, but we would like to avoid complexity of managing several versions of interop packages, especially for incremental changes.

Looking through the interfaces, do we actually need something like queryAllLocks that's only used for testing?

kabentley commented 1 year ago

Looking through the interfaces, do we actually need something like queryAllLocks that's only used for testing?

Probably not. In fact, all of the api dealing with locks should probably have been a separate interface.

The current question is that this whole interface is declared to be @beta. It should either go to @public or @internal. I personally think it should be @internal.

calebmshafer commented 1 year ago

@nick4598 this is marked as 3.6. Did we get the changes we wanted in there or 3.7?

If not, we need to figure out our plan for 4.0 as this is where the issue started.

nick4598 commented 1 year ago

Bill and I agree that internal is probably the better way to go here. We don't want anyone to use these functions besides the iTwin.js libraries so we should keep them internal. @calebmshafer , This change would go into master, as well as backported to release/3.7.x branch correct?

calebmshafer commented 1 year ago

@nick4598 that sounds fine to me. Let's just get this in for 4.0 so master only would work.