Skip to content
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

Merged
merged 2 commits into from
Sep 29, 2023

Conversation

DaddyWarbucks
Copy link
Contributor

Solves: #2050

This PR updates the object, array, and tuple schemas such that they are resolved before being described. This ensures that the describe method works for things like object({...}).when(...)

Copy link

@SpiderDan98 SpiderDan98 left a 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>) {
Copy link
Owner

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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
    ...
  }

Copy link
Contributor Author

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:

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

Copy link
Owner

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

Copy link
Contributor Author

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.

Copy link
Owner

@jquense jquense left a 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

@lukefredrickson
Copy link

@DaddyWarbucks thanks for working on this — much appreciated!

@lukefredrickson
Copy link

lukefredrickson commented Sep 27, 2023

@jquense @DaddyWarbucks Sorry to be a bother, but do you have an ETA for merging this work in?

@DaddyWarbucks
Copy link
Contributor Author

Thats above my pay grade.

@jquense jquense merged commit 020901f into jquense:master Sep 29, 2023
1 check passed
@DaddyWarbucks DaddyWarbucks deleted the bug/2050-object-describe branch October 10, 2023 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants