Skip to content

Conversation

@wparad
Copy link
Contributor

@wparad wparad commented Aug 7, 2022

Fixes a bunch of unnecessary complexity in recursion and two issues that should be resolved.

No idea why the browser tests are failing, I thought it was bad coveralls configuration, but after fixing that, it is not. The output says success, but github action says nope.

@wparad wparad force-pushed the fix-dereference-recursion branch from 7bb4ffd to 3715979 Compare August 8, 2022 09:07
@wparad wparad force-pushed the fix-dereference-recursion branch from c50da33 to 3286ac7 Compare August 8, 2022 13:31
@jonluca
Copy link
Collaborator

jonluca commented Feb 3, 2023

I don't believe the new behavior is correct.

For https://github.com/APIDevTools/json-schema-ref-parser/pull/277/files#diff-e17696fc5b8288ef1f42c8848360d8b90d660f7b3606465f458c25b0174b1e0aL66 when circular is set to ignore, it should not be re-creating a partial definition - it should keep the $ref as is

@wparad
Copy link
Contributor Author

wparad commented Feb 20, 2023

I don't believe the new behavior is correct.

For https://github.com/APIDevTools/json-schema-ref-parser/pull/277/files#diff-e17696fc5b8288ef1f42c8848360d8b90d660f7b3606465f458c25b0174b1e0aL66 when circular is set to ignore, it should not be re-creating a partial definition - it should keep the $ref as is

I think there is a lot to debate here. I'm not changing the behavior, I'm changing where the "circle" is detected. Right now it is all the way at the beginning, and as such we lose out on all the awesome data we collected. For instance if a cycle is 10 $ref deep,

0 => 1 => 2 => 3 => 4 => 5 => 6 => 7 => 8 => 9 => 0 => 1. Then the current result of the library is to show:

0 => $ref

That's terrible, it's far better to show
0 => 1 => 2 => 3 => 4 => 5 => 6 => 7 => 8 => 9 => 0 => $ref

Without this, it makes dereference next to useless, and requires anyone that wants to show valuable circular references to use bundle and build the real dereference solution themselves.

If we really don't want to change how ignore works, we can create a new property value, but that's honestly really dumb, since no one would want to use it and the "show ignore but also include all the valuable information" is so much better. There are also at least two other issues in this repo attempting to deal with this problem (although they never understood what the real problem was).

Also, the recursion is just broken. Nested and deep property overrides don't work. The caching incorrectly stores the $ref values instead of using the overrides.

I really want to merge this/improve it. How can we achieve that?

@jonluca
Copy link
Collaborator

jonluca commented Sep 18, 2023

That's very fair. I'm closing stale PRs - happy to merge in a new PR if it's rebased

We'll release it as a breaking change. We should also have an option to keep the old behavior

@jonluca jonluca closed this Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants