-
-
Notifications
You must be signed in to change notification settings - Fork 208
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: merged schemas cache key #729
base: master
Are you sure you want to change the base?
fix: merged schemas cache key #729
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.
This should work, i don't know about the performance of that package, but this could be a lot faster way of setting unique identifiers:
const idMap = new WeakMap();
const getId = (obj) => {
if (!idMap.has(obj)) {
idMap.set(obj, Symbol('id'));
}
return idMap.get(obj);
};
const id = getId(object);
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
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
I'm a bit scared of this as it would likely increase our build/startup time.
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.
thank you for your PR. I'm rejecting it for now, because I remember I was using objects pointers as keys on purpose to compare objects when going through the refs cycles. I need a bit more time to look at this PR. I will try to review asap.
I found the problem. I'm working on the solution. Unfortunatelly merging schemas with recurcive references is the most complecated part of this library and take tons of time every time when we need to fix the bug. |
npm run test
andnpm run benchmark
documentation is changed or addedand the Code of conduct
Hello, this PR fixes #724.
When diving in the bug, I found that the
context.mergedSchemasIds
Map, added by @ivan-tymoshenko, is using objects as keys. This caused the schema to infinitely try to build and results intoMaximum call stack size exceeded
error.So I replaced the key by a hash of the object, then I got another problem :
anyOf inside allOf
test failed because, to my understanding, we try to cache different merged schemas using the same key incontext.mergedSchemasIds
(like merged schemas frombuildOneOf
and frombuildAllOf
functions).I am not sure of the solution. Thank you for your review !