Skip to content

fix: ObjectSchema.extend returns a schema instance #253

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

deepaknf
Copy link

@deepaknf deepaknf commented Sep 5, 2024

Summary

This PR addresses the issue #249 where ObjectSchema.extend Does not return a schema instance. The fix makes it return the schema.

Checklist

@bencoder
Copy link

bencoder commented Sep 5, 2024

It seems it was intentionally not returning a Schema instance before this. Do we know why it was like that in the first place?

Looking at where that was originally changed (to not return Schema) I see this PR & issue with some explanation but I don't understand the context well enough to say if it's still valid:

#69
#65

@kieranswhite
Copy link

@aboutlo I don't suppose you can help us understand this? It looks like you were heavily involved in the previous issues that @bencoder listed. Thanks :)

@simoneb
Copy link
Contributor

simoneb commented Sep 5, 2024

I don't think we can safely merge this change, precisely because, as observed, this behavior is intentional. The change is straightforward, but before we even change anything, we need to understand why the current behavior was intentionally implemented in this way, and the explanation lies in the PRs linked by @bencoder above. So, please have a look at the whole history before deciding what to do with this issue. A viable option is to close it as it works as designed, but I would consider giving another look at the original motivations behind this behavior, to see if there's a solution that wasn't considered back then.

@Fdawgs Fdawgs requested a review from Copilot May 2, 2025 07:18
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR addresses an issue where ObjectSchema.extend did not return a schema instance, causing unexpected chaining problems. The changes include:

  • Updating ObjectSchema.extend to return a complete fluent schema instance.
  • Adding a new test to verify chainability with the without() method.
  • Removing the test that expected an error when calling prop() after extend.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/ObjectSchema.test.js Introduces a test verifying chaining after extend works with without().
src/ObjectSchema.js Modifies extend() so that it returns the full schema instance, enabling seamless chaining.
Comments suppressed due to low confidence (1)

src/ObjectSchema.js:343

  • Returning a full schema instance from extend() improves chainability; please verify that upstream documentation and consumers are updated to reflect this API change.
return ObjectSchema({ schema: extended, ...options })

@esatterwhite
Copy link
Contributor

@aboutlo are you able to chime in on this?

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.

5 participants