Skip to content

feat: add onBundle callback #391

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

aeworxet
Copy link

@aeworxet aeworxet commented Jul 2, 2025

This PR adds functionality, as well as an options object, for the bundle method, which consists of:

  • The onBundle callback.
  • The excludedPathMatcher callback.

Closes #346

@aeworxet
Copy link
Author

aeworxet commented Jul 2, 2025

This functionality mimics the onDereference callback along with the test, which is likely why the test fails: the bundle method should have different test conditions.

@jonluca, could you please advise what the test should be?

auto-merge was automatically disabled July 2, 2025 10:16

Head branch was pushed to by a user without write access

@jonluca jonluca enabled auto-merge (squash) July 2, 2025 20:48
@jonluca
Copy link
Collaborator

jonluca commented Jul 2, 2025

Hello,

The logic looks sound, I think you just need to fix the test comparison. The bundle traversal is stable, right? The tests aren't failing based on platform, so just make sure yarn test returns 100% on your local machine.

@aeworxet
Copy link
Author

aeworxet commented Jul 3, 2025

Manual execution has shown that correct $refs are pushed to calls array, but vitest's expect operates on the changed $refs, which is why the test fails.
The debugger showed that the alteration of the $refs is the responsibility of the remap() function.
With remap() commented out, the $refs don't get altered before the app finishes execution, and the test passes.

I'm wary of saying that this is a bug in the remap() function because someone would already have noticed such an obvious difference in expected results, and, additionally, I get correct results when using my production app.

@jonluca, could you please clarify whether this behavior of remap() is specific to vitest and testing conditions, or if it is still a flaw in remap()'s execution?

},
{
path: "#/definitions/a",
value: { $ref: "#/definitions/apath: "#/definitions/b",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is missing a comma and closing quote

@jonluca
Copy link
Collaborator

jonluca commented Jul 3, 2025

That is not the issue, the issue is that there is a syntax error in your code. https://github.com/APIDevTools/json-schema-ref-parser/pull/391/files#r2181747158

The original error is that your deep expect is incorrect. https://github.com/APIDevTools/json-schema-ref-parser/actions/runs/16018336530/job/45189942210

If you fix the hardcoded object it should pass.

Please run the tests locally before submitting an update and verify they all succeed.

auto-merge was automatically disabled July 3, 2025 05:55

Head branch was pushed to by a user without write access

@aeworxet
Copy link
Author

aeworxet commented Jul 3, 2025

Apologies, I didn't push the test I was debugging.
With this version of the test, I get correct results with my production app, but a failing test with yarn test.

screenshots

image

image

@jonluca
Copy link
Collaborator

jonluca commented Jul 3, 2025

Awesome, can you remove the console log? Otherwise looks good

@aeworxet
Copy link
Author

aeworxet commented Jul 3, 2025

Removed.

@aeworxet
Copy link
Author

aeworxet commented Jul 3, 2025

There it is: a failed test.
With remap() commented out, the $refs don't get altered before the app finishes execution, and the test passes.

@jonluca
Copy link
Collaborator

jonluca commented Jul 3, 2025

Can you change the test to just calls.push(JSON.parse(JSON.stringify({ path, value, parent, parentPropName }))); instead, to keep it stable?

Modifying the refs is how the internal bundler works, it's up to the caller of the onBundle passer to know that. Or we could change the caller of onBundle to pass a new version of the objects as well.

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.

Possibility to add onBundle callback?
2 participants