Deprecate `SnapshotConnection.openRemote` and make `SnapshotConnection.openFile` IPC only
Public API changes
- Deprecate
SnapshotConnection.openRemoteAPI. - Make
SnapshotConnection.openFileonly available in IPC apps.
Motivation
SnapshotConnection relies on SnapshotIModelRpcInterface for it's communication with the backend. With deprecation and eventual replacement of RPC, all RPC interfaces owned by core need to reviewed and rewritten as needed. While some of the other RPC interfaces (e.g. IModelReadRpcInterface) will need to be converted to the new architecture, I believe, SnapshotIModelRpcInterface can be moved to IPC as it doesn't make sense to have Web implementation for it.
Details
SnapshotConnection / SnapshotIModelRpcInterface has two main methods:
-
openFile(filePath: string, ...)- opening iModel using file path only makes sense if backend is on the same machine. This is the case in Electron and mobile in which IPC can be used. For that reason newIpcApp.appFunctionIpc.openSnapshotwas added. -
openRemote(key: string, ...)- as far as I can tell, this was intended to be used in Web Viewer. Web Viewer and other iTwin.js web applications should be usingCheckpointConnection.openRemote(...). Since GPB doesn't have implementation registered forSnapshotIModelRpcInterface, this is not even usable in the web without custom backend.
TODO
- [x] Confirm if this is a breaking change or not.
- ~While changing
SnapshotConnection.openFileto work with IPC and not RPC could be considered a breaking change as it's excludes web applications from calling it, it's highly unlikely we're going to break anyone since someone needs to have a custom backend set up to useopenFilein the web application.~ - Some web apps use
SnapshotConnection.openFileto open iModels for tests. This change will break their tests.
- ~While changing
- [x] Find a way to replace
SnapshotConnection.openFilein full-stack-tests/core as we will want to keep running those tests in Chrome.- Added new
TestSnapshotConnectionclass which opensiModelDbthrough custom .
- Added new
- [x] Find a way to replace
SnapshotConnection.openFilein full-stack-tests/presentation or support IPC there.- Added new
TestIModelConnectionclass which opensiModelDbdirectly.
- Added new
- [x] Don't break display-test-app (usage).
- display-test-app has IPC available in all 3 environments (web, desktop and mobile).
- display-performance-test-app does not have IPC available in web, so added
TestSnapshotConnectionsimilar to the one in core full-stack-tests.
- [x] Update NextVersion.md.
- [x] Review and update documentation.
Please add to your TODO list: Don't break display-test-app. See usage here.
lgtm;
@markschlosseratbentley please review changes to DTA
@MarcNeely FYI
lgtm; @markschlosseratbentley please review changes to DTA
@MarcNeely FYI
@aruniverse I did some very basic testing (successfully opened an iModel in a DTA built off this branch) and it works. @MarcNeely may have some more thorough testing he can do.
This pull request is now in conflicts. Could you fix it @GytisCepk? 🙏 To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/
I did some very basic testing (successfully opened an iModel in a DTA built off this branch) and it works.
Did you confirm you can edit an iModel?
I did some very basic testing (successfully opened an iModel in a DTA built off this branch) and it works.
Did you confirm you can edit an iModel?
I did not confirm this.
EDIT: fyi @pmconne I have now confirmed that I can successfully edit an iModel using this branch. I placed a bunch of line strings in an iModel and those lines persisted.
I used Electron DTA.
This pull request is now in conflicts. Could you fix it @GytisCepk? 🙏 To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/
Make
SnapshotConnection.openFileonly available in IPC apps.
This will break integration tests of at least 1 application. I don't want to break anything on minor release, I'm moving this PR back to draft state. I'll reopen it once we're working on 5.0.
Separate PR will be opened soon to deprecate SnapshotConnection.openRemote.