Support for making references to schema properties

[techbech]

Fixes issue #617

[codecov]

Codecov Report

Merging #626 (8883175) into main (c09ace1) will increase coverage by 0.04%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##             main     #626      +/-   ##
==========================================
+ Coverage   93.52%   93.57%   +0.04%     
==========================================
  Files          11       11              
  Lines         510      529      +19     
  Branches      183      189       +6     
==========================================
+ Hits          477      495      +18     
- Misses         32       33       +1     
  Partials        1        1              

Impacted Files	Coverage Δ
src/load.ts	91.57% <50.00%> (-0.90%)	⬇️
src/transform/schema.ts	97.97% <100.00%> (+0.38%)	⬆️
src/utils.ts	94.82% <100.00%> (+0.09%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21c0561...8883175. Read the comment docs.

[drwpow]

Cool. This looks great, but I wouldn’t want to merge this without adding a test first. Can you write a unit test for this?

I think a great place would be in schema.test.ts, there is a $ref and $ref (external) test; maybe this could be a $ref (properties) test? Just in one place should be enough to make sure this is working as intended.

[drwpow]

Actually, on second thought, now that #602 is merged, we can keep your approach, but we’ll have to move it to load.ts#L152. Something like:

  let [base, ...rest] = parts;
+ if (rest[rest.length - 2] === 'properties') rest.splice(rest.length - 2, 1); // if "properties" is next-to-last segment, remove
  return `${base}["${rest.join('"]["')}"]`;

The change was due to the fact that because we resolve remote schemas, we have to resolve $refs at the very beginning in order to figure out what to load. And we have to transform them at the very beginning, too, because we only have that context during resolution. But other than that, this should look familiar to your approach here; it just moved to a different spot in the codebase.

[techbech]

Cool. This looks great, but I wouldn’t want to merge this without adding a test first. Can you write a unit test for this?

Sure

Actually, on second thought, now that #602 is merged, we can keep your approach, but we’ll have to move it to load.ts#L152.

All right, I will move it 🙂

[drwpow]

Just checking in. Are you still able to make this change? Then we can get this merged.

Sorry again for the v4 disruption on this!

[drwpow]

Approved pending moving to load.ts (as this utility no longer exists)

[techbech]

I chose to make the test as a schema-load-test in v2 and v3. That is because the check and change is made at the load phase, and the test in schema.test.ts is not for tests in the loading phase.

And sorry about the late response, I've been busy 🙂

[drwpow]

I chose to make the test as a schema-load-test in v2 and v3. That is because the check and change is made at the load phase, and the test in schema.test.ts is not for tests in the loading phase.

And sorry about the late response, I've been busy 🙂

No worries at all! I totally understand—I only work on this project when I have time as well. 🙂

[avaly]

Is there anything else blocking merging this PR?

I also ran into this issue recently and would like to get this patch available.

[drwpow]

Is there anything else blocking merging this PR?

I also ran into this issue recently and would like to get this patch available.

Nope! Sorry—I fell a little behind on maintenance for this project and am just now catching up. Will publish this in the next release!

[drwpow]

@all-contributors please add @Peteck for code, bug

[allcontributors]

@drwpow

I've put up a pull request to add @Peteck! 🎉