-
Notifications
You must be signed in to change notification settings - Fork 929
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
Allow parent resolve before describe. #2051
Allow parent resolve before describe. #2051
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.
Thanks for the pretty quick fix
src/array.ts
Outdated
@@ -283,6 +283,11 @@ export default class ArraySchema< | |||
} | |||
return base; | |||
} | |||
|
|||
describe(options?: ResolveOptions<TContext>) { |
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.
can we combine the two here, and just keep the one override?
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.
Perhaps, but I struggled to understand how. I started off with something like
describe(options?: ResolveOptions<TContext>) {
const next = options ? this.resolve(options) : this.clone();
let base = next.describe(options) as SchemaObjectDescription;
...
}
But the problem is the .resolve()
and .clone()
both return a new schema. So that following next.describe()
causes an infinite loop.
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.
I could do this
describe(options?: ResolveOptions<TContext>) {
const next = options ? this.resolve(options) : this.clone();
let base = Schema.prototype.describe.call(next, options) as SchemaObjectDescription
...
}
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.
Another solution would be to remove this line:
Line 908 in 1178437
const next = (options ? this.resolve(options) : this).clone(); |
And then add a
describe
method to every type that does the resolve/clone itself before calling super.describe()
. This is explicit and seems OK too.
Which doe you prefer @jquense?
- Use
describe
and_describe
as the code is now - Use
Schema.prototype.describe.call
- Define and explicit
describe
method on each type that is responsible for resolving itself
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 about:
const next = this.resolve(options)
const base = this.withMutation(() => {
return next.describe() // no options, shouldn't resolve
})
...
withMutation
could be removed i am not sure why the root describe calls clone()
maybe just to be safe with handing out mutable schema internals
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.
I believe calling next.describe()
would cause an infinite loop. I think I have a good solution, and I have updated the PR @jquense.
The code now resolves these types twice in the describe
method. For example
const next = (options ? this.resolve(options) : this).clone();
const base = super.describe(options) as SchemaInnerTypeDescription;
Then we can use next.whatever
instead of this.whatever
to use the new resolved schema to add additional information to the description. I'm not really concerned about any performance overhead there. I don't think the describe
method is generally being used in performance critical areas, and the clone
and resolve
should be fast anyway.
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.
yeah this is nice straightforward, works for me
@DaddyWarbucks thanks for working on this — much appreciated! |
@jquense @DaddyWarbucks Sorry to be a bother, but do you have an ETA for merging this work in? |
Thats above my pay grade. |
Solves: #2050
This PR updates the
object
,array
, andtuple
schemas such that they are resolved before being described. This ensures that thedescribe
method works for things likeobject({...}).when(...)