-
-
Notifications
You must be signed in to change notification settings - Fork 240
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
fix(resolving): detect the source of local references properly #615
Conversation
Hm, some weird issue with bundling 🤔 |
28f8b65
to
5fc81e9
Compare
5fc81e9
to
6986b82
Compare
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.
One question, but looks good. Best we can do for now - it's a tricky problem of how to report errors that occur in $ref'd targets.
@@ -48,12 +72,12 @@ export class Resolved { | |||
|
|||
if (target && target[REF_METADATA]) { | |||
return { | |||
path: [...get(target, [REF_METADATA, 'root'], []), ...newPath], | |||
path: [...get(target, [REF_METADATA, 'root'], []).map(decodePointerFragment), ...newPath], |
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.
Why is decodePointerFragment
required now?
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.
So the path stored under get(target, [REF_METADATA, 'root']
is escaped, meaning /foo
is represented as ~1foo
.
The path we expect to return should be unescaped (/foo
), so that the YAML/JSON getLocationForJsonPath can find a node correctly.
If you revert the change, the test I added will fail - produced ranges will be less precise (they will point at paths
rather than paths//test/get
.
Fixes #572 (comment) and a potential issue (see 28f8b65)
Checklist
Does this PR introduce a breaking change?