Skip to content

Conversation

@sdirix
Copy link
Member

@sdirix sdirix commented Jun 10, 2021

Removes json-schema-ref-parser and corresponding resolving functionality
and dependencies from JSON Forms.

Resolving of external dependencies is not a core feature of JSON Forms
which is why we delegated it to a 3rd party dependency. However the
json-schema-ref-parser library is not only slow and mutating schemas in
place but also brings in a number of questionable Node dependent
dependencies.

The only binding actually using this functionality is the React
binding. The way resolving was implemented resulted in the same flow
as resolving it outside of JSON Forms and handing the
resolved schema over.

Based on these downsides and the very little upsides we decided to
remove json-schema-ref-parser without a replacement from JSON Forms.

Angular and Vue consumers don't need to spend any migration efforts. In
@jsonforms/react we still export the backward API compatible
'ResolvedJsonFormsDispatch'. Therefore only React consumers actually
using the now removed 'refParserOptions' are forced to migrate while
for all other React consumers it depends on their use cases.

The built-in resolving mechanism in JSON Forms is quite basic. When
the schema was fully resolved anyway the basic resolving did quite
fine. However as this is no longer the case by default, there are
now cases which are no longer supported and break. This is especially
the case for complex combinator use cases involving references.

Of course these can still be used when the consumer resolves their
schema themselves before handing it over to JSON Forms. We also plan
to make our built-in resolving more robust before the full 3.0 release
to cover more of these cases. However we will not reach feature parity
without major additional efforts.

@sdirix sdirix force-pushed the remove-json-schema-ref-parser branch from 3f57522 to 45c2f65 Compare June 14, 2021 10:08
@coveralls
Copy link

coveralls commented Jun 14, 2021

Coverage Status

Coverage increased (+0.4%) to 88.663% when pulling 5e01daf on sdirix:remove-json-schema-ref-parser into b4f70e3 on eclipsesource:master.

@sdirix sdirix force-pushed the remove-json-schema-ref-parser branch from 45c2f65 to ae1e113 Compare June 14, 2021 10:24
@sdirix sdirix marked this pull request as ready for review June 14, 2021 10:49
@sdirix sdirix requested a review from eneufeld June 14, 2021 12:39
@sdirix sdirix linked an issue Jun 14, 2021 that may be closed by this pull request
});

it('should add an item within an array', async () => {
it.skip('should add an item within an array', async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why skip all these tests?

@eneufeld
Copy link
Member

I tested the examples, the AnyOf, OneOf, AllOf Resolve example (/packages/examples/src/anyOf-oneOf-allOf-resolve.ts) is crashing.
I also think that the packages/examples/src/resolve.ts is not useful.
The name: 'issue-1253-add-button-empty-row', example crashes.

I didn't test which of these crashed before.

Copy link
Member

@eneufeld eneufeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, we should update the resolver code and the examples in a follow up commit.

Removes json-schema-ref-parser and corresponding resolving functionality
and dependencies from JSON Forms.

Resolving of external dependencies is not a core feature of JSON Forms
which is why we delegated it to a 3rd party dependency. However the
json-schema-ref-parser library is not only slow and mutating schemas in
place but also brings in a number of questionable Node dependent
dependencies.

The only binding actually using this functionality is the React
binding. The way resolving was implemented resulted in the same flow
as resolving it outside of JSON Forms and handing the
resolved schema over for all but the very niche use case of unresolved
schemas in the ui schema registry. Even that case can still be handled
using a custom renderer.

Based on these downsides and the very little upsides we decided to
remove json-schema-ref-parser without a replacement from JSON Forms.

Angular and Vue consumers don't need to spend any migration efforts. In
@jsonforms/react we still export the backward API compatible
'ResolvedJsonFormsDispatch'. Therefore only React consumers actually
using the now removed `refParserOptions` are forced to migrate while
for all other React consumers it depends on their use cases.

The built-in resolving mechanism in JSON Forms is quite basic. When
the schema was fully resolved anyway the basic resolving did quite
fine. However as this is no longer the case by default, there are
now cases which are no longer supported and break. This is especially
the case for complex combinator use cases involving references.

Of course these can still be used when the consumer resolves their
schema themselves before handing it over to JSON Forms. We also plan
to make our built-in resolving more robust before the full 3.0 release
to cover more of these cases. However we will not reach feature parity
without major additional efforts.
@sdirix sdirix force-pushed the remove-json-schema-ref-parser branch from ae1e113 to 5e01daf Compare June 16, 2021 12:36
@sdirix sdirix merged commit f760975 into eclipsesource:master Jun 16, 2021
@sdirix sdirix deleted the remove-json-schema-ref-parser branch June 16, 2021 13:05
@sdirix sdirix linked an issue Jun 16, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Angular 12 compatibility Remove external ref resolving

3 participants