#179 Consistently use URI object in APIs #121

ndoschek · 2022-11-09T16:28:18Z

Consistently use URI object in APIs

Use urijs as typed URI library
Use URI objects in APIs and internally for proper URI handling
- Adapt interfaces, tests
Customize JsonRpcProxyFactory to ensure URI object is correctly typed onRequest/onNotifciation
- Configure on backend and frontend for the JsonRpc WebSocketConnection
Rename modeluri to lowercase where applicable for better alignment
Differentiate URI and TheiaURI object
Update README examples

Update dev-example to ModelServerClient V2

Update custom DevModelServerClient
Update api-test-menu
- User URIs to trigger commands
- Refine notification printing
- Add test commands that edit via JSON patch commands
- Remove xmi test commands as they are not relevant anymore

General

Remove simple-import-sort eslint plugin as it interferes with the built-in import sort
Ignore zipped rolling log files
Fix launch configs

Part of eclipse-emfcloud/emfcloud#179

Contributed on behalf of STMicroelectronics

- Remove simple-import-sort eslint plugin as it interferes with the built-in import sort - Ignore zipped rolling log files - Fix launch configs Contributed on behalf of STMicroelectronics

- Use `urijs` as typed URI library - Use URI objects in APIs and internally for proper URI handling - Adapt interfaces, tests - Customize JsonRpcProxyFactory to ensure URI object is correctly typed onRequest/onNotifciation - Configure on backend and frontend for the JsonRpc WebSocketConnection - Rename modeluri to lowercase where applicable for better alignment - Differentiate URI and TheiaURI object - Update README examples Part of eclipse-emfcloud/emfcloud#179 Contributed on behalf of STMicroelectronics

- Update custom DevModelServerClient - Update api-test-menu - User URIs to trigger commands - Refine notification printing - Add test commands that edit via JSON patch commands - Remove xmi test commands as they are not relevant anymore Contributed on behalf of STMicroelectronics

eneufeld

Code looks good overall. I have some questions and remarks, though.

examples/dev-example/src/browser/api-test-menu.ts

eneufeld · 2022-11-09T17:03:23Z

examples/dev-example/src/node/model-server-client.ts

    private intervalId: NodeJS.Timeout;

-    subscribe(modeluri: string, options: SubscriptionOptions = {}): void {
+    subscribe(modeluri: URI, listener?: SubscriptionListener, options: SubscriptionOptionsV2 = {}): SubscriptionListener {


We had multiple examples where the options were set but the listener was undefined. Why use subscription options if no listener is set anyway? This happened in examples/dev-example/src/browser/api-test-menu.ts

Previously with v1, the listener only was a member of the SubscriptionOptions. With v2 it should be a separate argument (for compatibility reasons, the options member will be used as the listener internally then).

Why use subscription options if no listener is set anyway?

Yes I agree that it might look a bit off, but in the case of the api-test-menu example we make use of a ModelServerSubscriptionClient that is initalized once and shows notifications received by opened subscriptions.

Perhaps we could re-work the example application a bit (in a separate ticket) and align the listener usage and also show the request (data like command or patch).

Could you please open a follow up here as suggested by you?

Follow-up ticket: #124

examples/dev-example/src/node/model-server-client.ts

examples/dev-example/src/browser/api-test-menu.ts

packages/modelserver-client/src/model-server-client-v2-integration.spec.ts

packages/modelserver-client/src/model-server-client-v2.spec.ts

- Use URI.equals() for uri comparison checks in tests - Revert unwanted changes in keepAliveInterval in CustomDevModelServerClient - Revert unwanted changes in api-test-menu

cdamus

Looks great! Thanks. I have just a few nits to remark on, but possibly not significant enough to need changes.

examples/coffee-theia/src/node/backend-module.ts

cdamus · 2022-11-10T20:00:21Z

packages/modelserver-client/src/model-server-client-api-v1.ts

@@ -28,7 +30,7 @@ export interface ModelServerClientApiV1 {
     * @param defaultFormat Optional fallback format that should used when a request method is invoked and no explicit format argument
     *                      has been passed.
     */
-    initialize(baseUrl: string, defaultFormat?: string): void | Promise<void>;
+    initialize(baseUrl: URI, defaultFormat?: string): void | Promise<void>;


I'm not sure about using the URI type for the HTTP resource/request URL. It's not really exactly an URI. Perhaps instead this should be the Node built-in URL type. What do you think?

I agree, the baseUrl is actually an URL (identifying the endpoint location as our jsdoc states) and not an URI, so using the URL type makes sense here.

I would like to stick to one type, that makes things a little more unified. Also the documentation of the URI library speaks of URLs being a "subset" of the URIs according to definition: https://medialize.github.io/URI.js/about-uris.html

Yes URLs are a subset. In the end in doesn't matter. I agree that for code simplification it make sense to stick to one type.

packages/modelserver-client/src/model-server-client-api-v2.ts

cdamus · 2022-11-10T20:09:13Z

packages/modelserver-client/src/model-server-client-v2-integration.spec.ts

@@ -234,7 +240,7 @@ describe('Integration tests for ModelServerClientV2', () => {
            const notification = await patchNotification;
            const patch = notification.patch;

-            expect(notification.modelUri).to.be.equal(modeluri);
+            expect(notification.modeluri.equals(modeluri)).to.be.true;


My only quibble with this is that it loses the presentation of the string comparison when the test fails, which is highly informative in the test report.

Is there a good reason not to test equality of the toString() conversions of the URIs? I wouldn't go so far as to implement a Chai assertion just for equality testing of URIs ... 😀

(there are many more occurrences of this throughout the tests)

Looking at the docs, the equals method does things like normalization and also ignores the order of query parameters. This is probably not needed here in the tests, but still cleaner imho.
The expect accepts a second parameter, which we should use to provide an error message which is helpful.
@cdamus, @ndoschek what do you think?

Yes I agree, thanks for that hint, I didn't think of that earlier, I will revert the changes from the first review cycle and go back to string comparisons for better test output.

why not use the second parameter for a better failure error?

Oh sorry, I didn't see your comment earlier. As discussed we will stick to the string comparisons for now and will create dedicated URI tests in a follow up (see #123 ) where we should test all varieties (e.g. relative hierarchies, random order of query parameters).

packages/modelserver-client/src/model-server-client-v2-integration.spec.ts

- Revert URI.equals() checks and use sring comparison checks in tests for better test result output - Rename variable to `patch` in model-server-client-v2-integration.spec.ts

eneufeld

Thank you, looks good.

ndoschek added 3 commits November 9, 2022 17:26

Housekeeping

b30100b

- Remove simple-import-sort eslint plugin as it interferes with the built-in import sort - Ignore zipped rolling log files - Fix launch configs Contributed on behalf of STMicroelectronics

ndoschek requested a review from cdamus November 9, 2022 16:30

eneufeld reviewed Nov 9, 2022

View reviewed changes

Address review comments

8413874

- Use URI.equals() for uri comparison checks in tests - Revert unwanted changes in keepAliveInterval in CustomDevModelServerClient - Revert unwanted changes in api-test-menu

ndoschek requested a review from eneufeld November 10, 2022 12:53

cdamus reviewed Nov 10, 2022

View reviewed changes

Address review comments #2

bc82a1b

- Revert URI.equals() checks and use sring comparison checks in tests for better test result output - Rename variable to `patch` in model-server-client-v2-integration.spec.ts

ndoschek force-pushed the issues/179 branch from dd89756 to bc82a1b Compare November 11, 2022 10:11

ndoschek mentioned this pull request Nov 11, 2022

Optimize Theia test application to show all features of V2 #124

Open

eneufeld approved these changes Nov 11, 2022

View reviewed changes

ndoschek merged commit 6238c88 into master Nov 11, 2022

ndoschek deleted the issues/179 branch November 11, 2022 10:40

ndoschek mentioned this pull request Jan 19, 2023

Add v2 requests to example application. #93

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#179 Consistently use URI object in APIs #121

#179 Consistently use URI object in APIs #121

ndoschek commented Nov 9, 2022

eneufeld left a comment

eneufeld Nov 9, 2022

ndoschek Nov 10, 2022

eneufeld Nov 10, 2022

ndoschek Nov 11, 2022

cdamus left a comment

cdamus Nov 10, 2022

eneufeld Nov 10, 2022

ndoschek Nov 11, 2022

eneufeld Nov 11, 2022

cdamus Nov 10, 2022

eneufeld Nov 10, 2022

ndoschek Nov 11, 2022

eneufeld Nov 11, 2022

ndoschek Nov 11, 2022

eneufeld left a comment

#179 Consistently use URI object in APIs #121

#179 Consistently use URI object in APIs #121

Conversation

ndoschek commented Nov 9, 2022

eneufeld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdamus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eneufeld left a comment

Choose a reason for hiding this comment