Add additional interfaces for Scoped, Labelable and Labeled ui elements. #1935
Conversation
Make Scopable scope optional and use Scoped with required scope where required. Add type predicates for Scopeable, Scoped, Labelable and Labeled. Duplicate old functions handling Scopeable to have Scopeable and Scoped where required. Update toDataPathSegments to allow nullable schemaPath and return an empty array if this happens.
|
|
There was a problem hiding this comment.
Thanks for the pull request!
In general it definitely makes sense to me to improve our typings whenever possible.
Can you explain your motivation for this change? Especially refactoring a long standing existing interface like Scopable which could introduce backward compatibility problems.
packages/core/src/models/uischema.ts
Outdated
| /** | ||
| * Interface for describing an UI schema element that may reference | ||
| * a subschema. The value of the scope may be a JSON Pointer or null. | ||
| */ | ||
| export interface Scopable { | ||
| /** | ||
| * The scope that determines to which part this element may be bound to. | ||
| */ | ||
| scope?: string; | ||
| } | ||
|
|
There was a problem hiding this comment.
If null is a foreseen value then this should be reflected in the typings, i.e. scope?: string | null. However we don't have any other nullebale types in JSON Forms, so I'm a bit hesitant to introduce this now. Is there a specific use case you have in mind?
This new interface with the optional scope is only used in one place: The new replacement for Scopable called Scoped. What is the use case of an element with optional scope?
There was a problem hiding this comment.
The case for this change would be the option to add scopes to existing classes without breaking the API in the future. This allows adding the interface to Categorization and Category in a second step to enable relative scopes in their children.
packages/core/src/models/uischema.ts
Outdated
| export const isScopeable = (obj: object): obj is Scopable => | ||
| obj !== undefined && obj.hasOwnProperty('scope'); |
There was a problem hiding this comment.
This does not fit. In Scopable the scope is optional. So why check for it here?
There was a problem hiding this comment.
You are right it. The check is only useful if was
interface Scopable {
scope: string|null;
}
toDataPathSegments: check for undefined, null and ''. null is not allowed by function definition, but it's included. Remove unnecessary type guards and improve the remaining ones.
There was a problem hiding this comment.
I talked with the team. From our point of view it makes sense to support Scopable, Scoped, Labelable and Labeled. Including the backward compatibility break of changing the Scopable interface. We're shortly before the 3.0 release, so that's the perfect point to get this change still in.
If you clean up the PR to only include the interface changes and the type guards we're going to merge it 👍
Replace Scopeable by Scoped where used Add type guards for Scoped and Labeled Make composeWithUI handle Scoped and Scopeable
packages/core/src/models/uischema.ts
Outdated
| @@ -249,7 +260,13 @@ export const isLayout = (uischema: UISchemaElement): uischema is Layout => | |||
| (uischema as Layout).elements !== undefined; | |||
|
|
|||
| export const isScopeable = (obj: object): obj is Scopable => | |||
| obj !== undefined && obj.hasOwnProperty('scope') && (obj as Scopable).scope !== undefined; | |||
| obj !== undefined && obj.hasOwnProperty('scope'); | |||
There was a problem hiding this comment.
scope is optional in Scopeable so it's allowed to be not set, therefore we should not check whether that property exists. Also we require the parameter to be an object so it can't be undefined.
I would like to suggest to improve the usefulness of the guard by marking the parameter as unknown and do some simple checks, i.e.
export const isScopeable = (obj: unknown): obj is Scopable =>
obj && typeof obj === 'object';
This checks that obj is neither undefined nor null and it's something which we can assign scope to.
packages/core/src/util/path.ts
Outdated
| if (!isScoped(scopableUi)) { | ||
| return ''; | ||
| } |
There was a problem hiding this comment.
Seems weird to return '' here when later in the code we return path when the scope was empty.
The could could be clearer by
export const composeWithUi = (scopableUi: Scopable, path: string): string => {
if (!isScoped(scopableUi)) {
return path ?? '';
}
const segments = toDataPathSegments(scopableUi.scope);
if (isEmpty(segments)) {
return path ?? '';
}
return compose(path, segments.join('.'));
};
packages/core/src/models/uischema.ts
Outdated
| isScopeable(obj) && obj.scope !== undefined; | ||
|
|
||
| export const isLabelable = (obj: object): obj is Lableable => | ||
| obj !== undefined && obj.hasOwnProperty('label'); |
There was a problem hiding this comment.
This check should be adapted similarly to Scopable
use typeof obj.scope to check for Scoped Co-authored-by: Stefan Dirix <sdirix@eclipsesource.com>
Make Scopable scope optional and use Scoped with required scope where required. Add type predicates for Scopeable, Scoped, Labelable and Labeled. Duplicate old functions handling Scopeable to have Scopeable and Scoped where required. Update toDataPathSegments to allow nullable schemaPath and return an empty array if this happens.
toDataPathSegments: check for undefined, null and ''. null is not allowed by function definition, but it's included. Remove unnecessary type guards and improve the remaining ones.
Replace Scopeable by Scoped where used Add type guards for Scoped and Labeled Make composeWithUI handle Scoped and Scopeable
use typeof obj.scope to check for Scoped Co-authored-by: Stefan Dirix <sdirix@eclipsesource.com>
Make Scopable scope optional and use Scoped with required scope where required.
Add type predicates for Scopeable, Scoped, Labelable and Labeled.
Duplicate old functions handling Scopeable to have Scopeable and Scoped where required.
Update toDataPathSegments to allow nullable schemaPath and return an empty array if this happens.