Deprecate `SnapshotConnection.openRemote` and make `SnapshotConnection.openFile` IPC only

GytisCepk commented 1 week ago

Public API changes

Deprecate SnapshotConnection.openRemote API.
Make SnapshotConnection.openFile only 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 new IpcApp.appFunctionIpc.openSnapshot was 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 using CheckpointConnection.openRemote(...). Since GPB doesn't have implementation registered for SnapshotIModelRpcInterface, 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.openFile to 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 use openFile in the web application.
[x] Find a way to replace SnapshotConnection.openFile in full-stack-tests/core as we will want to keep running those tests in Chrome.
- Added new TestSnapshotConnection class which opens iModelDb through custom .
[x] Find a way to replace SnapshotConnection.openFile in full-stack-tests/presentation or support IPC there.
- Added new TestIModelConnection class which opens iModelDb directly.
[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 TestSnapshotConnection similar to the one in core full-stack-tests.
[x] Update NextVersion.md.
[x] Review and update documentation.

pmconne commented 1 week ago

Please add to your TODO list: Don't break display-test-app. See usage here.

markschlosseratbentley commented 2 days ago

lgtm;

@markschlosseratbentley please review changes to DTA

@MarcNeely FYI

markschlosseratbentley commented 2 days ago

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.

mergify[bot] commented 1 day ago

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/

pmconne commented 1 day ago

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?

markschlosseratbentley commented 1 day ago

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.

iTwin / itwinjs-core