-
-
Notifications
You must be signed in to change notification settings - Fork 205
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
$ref support #12
$ref support #12
Conversation
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.
Good work, one minor issue on the use of require
.
// If external file | ||
if (ref[0]) { | ||
schema = require(ref[0]) | ||
} |
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.
Never do this, it's sync and done in a place where it might cause to bugs. We should not load files into Node. However, we should use an approach similar to https://www.npmjs.com/package/is-my-json-valid#external-schemas.
for (let i = 1; i < walk.length; i++) { | ||
code += `['${walk[i]}']` | ||
} | ||
return (new Function('schema', code))(schema) |
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.
Nice!!
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.
Great work!!!!
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.
LGTM
As titled.
This implementation supports both internal and external definitions.
Caveats:
If the user is using an external json file for his definitions it must use
__dirname
to get the correct path.See the following example (the same of the test).
If everything is ok to you I can update the docs.