-
Notifications
You must be signed in to change notification settings - Fork 196
fix: normalize explicit self-file $ref to internal pointer #2356
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
Conversation
🦋 Changeset detectedLatest commit: 83c545d The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
tatomyr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of minor suggestions. Also, could you modify some tests to first point to another file and then use self reference inside of it (just to be sure it works not only for root files)?
| license: | ||
| name: MIT | ||
| servers: | ||
| - url: http://example.com | ||
| security: [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep the test description minimal to only showcase the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need license, summary, operationId.
| extends: | ||
| - recommended |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you need this for?
| title: AsyncAPI 2 self-file ref | ||
| version: '1.0.0' | ||
| channels: | ||
| c: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use the naming consistently here and in other tests (big/small letters; using/not using number)? Preferably something like we already use in our examples (TestChannel/foo/...).
|
One more thing: please make sure to test cases mentioned in this comment. |
…r OAS2 and AsyncAPI 3
tatomyr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving with minor suggestions.
| license: | ||
| name: MIT | ||
| servers: | ||
| - url: http://example.com | ||
| security: [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need license, summary, operationId.
packages/core/src/bundle.ts
Outdated
| } | ||
|
|
||
| // Normalize explicit self-file refs to internal pointer | ||
| if (ctx.location.source.absoluteRef === resolved.location.source.absoluteRef) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it still work if we change it this way?
| if (ctx.location.source.absoluteRef === resolved.location.source.absoluteRef) { | |
| if (ctx.location.source === resolved.location.source) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or the location pointers are actually different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tatomyr moved the link replacement to the check below
packages/core/src/bundle.ts
Outdated
| !dereference | ||
| ) { | ||
| // Normalize explicit self-file refs to internal pointer | ||
| node.$ref = resolved.location.pointer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check if comparing those values first will make performance better:
| node.$ref = resolved.location.pointer; | |
| if (node.$ref !== resolved.location.pointer) { | |
| node.$ref = resolved.location.pointer; | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.

What/Why/How?
Normalize explicit self-file $ref to local internal pointers during bundling and avoid external loading for self-references in resolver.
Becomes:
Reference
fix #1597
Testing
packages/core/src/__tests__/bundle.test.ts__tests__/bundle/self-file-ref-normalization.Screenshots (optional)
Check yourself
Security