itwinjs-core icon indicating copy to clipboard operation
itwinjs-core copied to clipboard

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

Open GytisCepk opened this issue 1 year ago • 8 comments

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.~
    • Some web apps use SnapshotConnection.openFile to open iModels for tests. This change will break their tests.
  • [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.

GytisCepk avatar Sep 18 '24 11:09 GytisCepk

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

pmconne avatar Sep 18 '24 11:09 pmconne

lgtm;

@markschlosseratbentley please review changes to DTA

@MarcNeely FYI

markschlosseratbentley avatar Sep 26 '24 18:09 markschlosseratbentley

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.

markschlosseratbentley avatar Sep 26 '24 19:09 markschlosseratbentley

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/

mergify[bot] avatar Sep 27 '24 13:09 mergify[bot]

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?

pmconne avatar Sep 27 '24 15:09 pmconne

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.

markschlosseratbentley avatar Sep 27 '24 16:09 markschlosseratbentley

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/

mergify[bot] avatar Oct 01 '24 09:10 mergify[bot]

Make SnapshotConnection.openFile only 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.

GytisCepk avatar Oct 01 '24 10:10 GytisCepk