Remove $id dependency from property util #1052

edgarmueller · 2018-08-08T13:29:27Z

This is a 1st attempt at removing the assumption that relevant subschemas have to be annotated with $id when using the TreeWithDetail renderer. These changes might cause the renderer to be somewhat slower as we can not lookup the container properties anymore, but instead re-calculate them. It also depends on a deep equal comparison of the involved schemas. Tthe modelMapping, label and image provider are pure functions now. Declarative maps can still be provided by creating helpers around these functions, e.g. we could do so in the theia-tree-editor extension. I believe these changes have simplified the code a bit, and we can still re-introduce lookups later as an optimization step, in case the schema provides $ids.

Relevant changes also include:

Move traverse to common util
Remove reference service (if this is in use anywhere it should be moved there, this is currently dead code)
Export UISchemaTester type
Remove treeWithDetail/editor reducer
Use UI schemas state to register detail UI schemas
Rename types in TreeWithDetailRenderer for consistency reasons

coveralls · 2018-08-08T13:44:06Z

Coverage increased (+0.7%) to 89.55% when pulling e81101e on edgarmueller:master into 14b21da on eclipsesource:master.

edgarmueller · 2018-08-08T13:44:15Z

packages/material-tree-renderer/src/services/property.util.ts

@@ -56,7 +53,7 @@ export const findPropertyLabel = (property: Property): string => {
    [
      property.schema.title,
      property.label,
-      property.schema.$id,
+      (property.schema as any).$id || (property.schema as any).id,


Since using id is not mandatory, should we really check this?

AhmetTanakol

I tried to look at it in detail. It is hard to make assumptions on efficiency but maybe we can try to cache matching schema. We can save the matching ones in a map so that before traversing we can check whether the schema that we are looking for exists in the map.

AhmetTanakol · 2018-08-09T12:03:39Z

packages/material-tree-renderer/src/services/property.util.ts

@@ -1,17 +1,14 @@
 import * as _ from 'lodash';
-import { JsonSchema7 } from '@jsonforms/core';
+import { JsonSchema } from '@jsonforms/core';


Are we using version 4? If yes, this might cause inconsistencies for the applications that depend on material-tree-renderer. As I remembered using version 7 in ui-schema-editor with material-tree-renderer which depends on version 4 was problematic.

JsonSchema is a union type of JsonSchema4 and JsonSchema7, so that should be fine I guess, or did I miss something?

AhmetTanakol · 2018-08-09T12:33:02Z

packages/material-tree-renderer/src/services/property.util.ts

+                            schema: JsonSchema,
+                            rootSchema: JsonSchema,
+                            isInContainer: boolean,
+                            visisted: Map<string, string>,


there is a typo

AhmetTanakol · 2018-08-09T12:42:59Z

packages/material-tree-renderer/src/services/property.util.ts

@@ -56,7 +53,7 @@ export const findPropertyLabel = (property: Property): string => {
    [
      property.schema.title,
      property.label,
-      property.schema.$id,
+      (property.schema as any).$id || (property.schema as any).id,


Since using id is not mandatory, should we really check this?

AhmetTanakol · 2018-08-09T12:54:34Z

packages/material-tree-renderer/example/index.ts

+];
+
+const filterPredicate = () => (property: Property): boolean =>
+  traverse(taskSchema, subSchema => subSchema === property.schema) !== undefined;


Maybe instead of ===, we can use _.isEqual() for deep comparison

This always returned false, I'm wondering why this actually worked.

Ah right, got it, traverse always returns a boolean (which is kinda weird, but that's a different story), hence the expression evaluated to true. I hopefully fixed this by tracking the schema path in findContainerProps as well and maintaining an additional property orgSchema in Property which is the reference to the sub schema of the root schema, so that we are able to compare by reference here.

eneufeld · 2018-08-13T07:34:01Z

I like the change, but I would still suggest to split it into at least two part, one being the refactoring of the label and image provider and a second being the change of the resolver.

* Remove $id properties from schema * Remove obsolete treeWithDetailReducer and init state * Use simple functions for providing labels and images * Remove config.ts * Remove resolving of $refs

edgarmueller · 2018-08-27T12:46:20Z

@eneufeld I've rebased my changes, please have a look again

eneufeld

looks good

eneufeld · 2018-08-27T14:48:38Z

packages/material-tree-renderer/example/index.ts

-          return label;
-        }
-      }
+  (schema: JsonSchema7) => (element: any): string => {


should we remove all the JsonSchema7 stuff and replace it with JsonSchema?

edgarmueller commented Aug 8, 2018

View reviewed changes

AhmetTanakol reviewed Aug 9, 2018

View reviewed changes

edgarmueller added 4 commits August 27, 2018 14:01

Remove $id dependency from property util

ebe33e1

Export UISchemaTester type

37d816f

Remove obsolete files from material-tree-renderer, minor fixes

ea910ff

Remove $id from material-tree-renderer example and simplify example

7d4265a

* Remove $id properties from schema * Remove obsolete treeWithDetailReducer and init state * Use simple functions for providing labels and images * Remove config.ts * Remove resolving of $refs

edgarmueller force-pushed the master branch from 4d4b0b1 to 7d4265a Compare August 27, 2018 12:09

eneufeld approved these changes Aug 27, 2018

View reviewed changes

Replace JsonSchema7 with more generic JsonSchema type in tree example

e81101e

edgarmueller merged commit c6e7f43 into eclipsesource:master Aug 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove $id dependency from property util #1052

Remove $id dependency from property util #1052

edgarmueller commented Aug 8, 2018 •

edited

Loading

coveralls commented Aug 8, 2018 •

edited

Loading

edgarmueller Aug 8, 2018

AhmetTanakol Aug 9, 2018

AhmetTanakol left a comment

AhmetTanakol Aug 9, 2018

edgarmueller Aug 9, 2018

AhmetTanakol Aug 9, 2018

AhmetTanakol Aug 9, 2018

AhmetTanakol Aug 9, 2018

edgarmueller Aug 9, 2018

edgarmueller Aug 9, 2018

eneufeld commented Aug 13, 2018

edgarmueller commented Aug 27, 2018

eneufeld left a comment

eneufeld Aug 27, 2018

Remove $id dependency from property util #1052

Remove $id dependency from property util #1052

Conversation

edgarmueller commented Aug 8, 2018 • edited Loading

coveralls commented Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AhmetTanakol 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

eneufeld commented Aug 13, 2018

edgarmueller commented Aug 27, 2018

eneufeld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edgarmueller commented Aug 8, 2018 •

edited

Loading

coveralls commented Aug 8, 2018 •

edited

Loading