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

feat: Adds property typing for an object attribute #336

Closed
wants to merge 17 commits into from

Conversation

whoami-pwd
Copy link
Contributor

@whoami-pwd whoami-pwd commented Jan 24, 2025

Description

This PR introduces refactoring and enhancements and the functionality for creating typed attribute objects by specifying in block.json.


Changes to Block.php and Child Classes

  • Constructor Refactoring:
    • Merged block_attributes directly in the constructor.
    • Changed the type of $block_attributes and $additional_block_attributes from array|null to array, defaulting to an empty array.
  • Property Optimization:
    • Removed the $block_attributes parameter from the get_block_attribute_fields method in favor of the block_attributes property defined in the constructor.
  • Method Renaming:
    • Renamed get_query_type to create_and_register_inner_object_type, reflecting its broader functionality.

Object Attribute Typing

  • Added support for specifying typing in block.json for object attributes. Example:
    "attributes": {
      "film": {
        "type": "object",
        "default": {
          "id": 0,
          "title": "Film Title",
          "director": "Director Name",
          "missing": "Make sure to define in typed" // This item will not be processed, due to it is missing from typed record.
          "__typed": {
            "id": "integer",
            "title": "string",
            "director": "string",
            "year": "string", // We can provide more items without actually providing default values.
            "somethingelse": "string" // We can provide more items without actually providing default values.
          }
        }
      }
    }

Refer to the updated README.md for details on the new functionality.

Readme instructions update

Copy link

changeset-bot bot commented Jan 24, 2025

🦋 Changeset detected

Latest commit: 39293c9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@wpengine/wp-graphql-content-blocks Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@whoami-pwd whoami-pwd marked this pull request as ready for review January 27, 2025 10:53
@whoami-pwd whoami-pwd requested a review from a team as a code owner January 27, 2025 10:53
@colinmurphy
Copy link
Contributor

@whoami-pwd Can we add some tests for an object to catch any breaking changes going forward. Otherwise the code looks great to me 😄

@theodesp
Copy link
Member

@justlevine thoughts of this proposal?

@justlevine
Copy link
Contributor

@justlevine thoughts of this proposal?

Not entirely sure I understand everything that's going on here or the purposes behind these seemingly unrelated things.

Two things that jumped out at me:

  1. Changed the type of $block_attributes and $additional_block_attributes from array|null to array.

    I'm pretty sure this is a breaking change to anyone who's been extending these classes, not entirely clear what in this PR justifies that break. Pretty sure this isnt the only breaking change either, but I didn't really dive into things.

  2. Where is the __typed property coming from?

    If it's a something we're inventing here and needs to be user-registered, then what justifies this approach vs using the existing APIs? (e.g but not only the aforementioned $additional_block_attributes)?

@theodesp
Copy link
Member

@justlevine thoughts of this proposal?

Not entirely sure I understand everything that's going on here or the purposes behind these seemingly unrelated things.

Two things that jumped out at me:

  1. Changed the type of $block_attributes and $additional_block_attributes from array|null to array.

    I'm pretty sure this is a breaking change to anyone who's been extending these classes, not entirely clear what in this PR justifies that break. Pretty sure this isnt the only breaking change either, but I didn't really dive into things.

  2. Where is the __typed property coming from?
    If it's a something we're inventing here and needs to be user-registered, then what justifies this approach vs using the existing APIs? (e.g but not only the aforementioned $additional_block_attributes)?

Hey @justlevine, thanks for your feedback! Let me address your concerns:

1. Changing array|null to array for $block_attributes and $additional_block_attributes

You're absolutely correct that changing the type from array|null to array is a breaking change. This could impact relying on the ability to pass null as a valid value.

While this is indeed a breaking change, I think overall it might be justified because allowing null can lead to ambiguity and potential runtime issues, as it requires additional checks to handle null cases. A default empty array ([]) is a safer and more consistent.

2. The __typed Property

The __typed property is a new feature designed to provide a structured way to define and enforce types for object properties in block attributes.

My understanding is that:

  • The __typed property is optional. Existing object types that don't include it will continue to work as before.
  • When __typed is present, the system validates the object properties against the defined types.

This should allow the user to query an object type by providing the __typed structure is defined in the block's JSON configuration.

@whoami-pwd maybe you can expand your views as well?

@whoami-pwd
Copy link
Contributor Author

@justlevine Thank you for your insightful comments!
And thank you @theodesp for expanding on the idea behind this PR!

@justlevine, you are absolutely correct that the commits prior to 6f0a3fa are largely focused on code refactoring. Beyond reducing nesting and simplifying the code with early returns to improve readability, there is a significant change to the property type. As you’ve astutely noted, this does impact child classes due to the property type modification.

However, based on the current implementation, I couldn’t identify any instances where these attributes are used as anything other than arrays. This change will enable us to directly leverage them as arrays, which I believe is a reasonable improvement.

Ideally, I would prefer to introduce these changes separately, outside the scope of this feature PR, to adhere to best practices for code retrospectives and refactoring. Unfortunately, we don’t yet have a formalized process for handling these types of changes independently. As a result, I’ve included them in this PR because, while not directly tied to the feature, they are related to the class where the feature changes are applied.

The __typed Property
The main purpose of this feature is to enable querying typed properties of the object type (Issue). Currently 'object' types are being processed as non-queryable scalar type objects. Could you please elaborate further on what you mean by using the existing APIs? I’d like to better understand your perspective and ensure we’re aligned.

@moonmeister
Copy link
Member

moonmeister commented Jan 29, 2025

It sounds like the refactoring needs to be a separate PR from the immediate feature if it's not required. Based on what @theodesp and @whoami-pwd have said, this breaking change is also not required. If that's the case, let's log an issue, and we can address it in the future. The constant breaking changes on this plugin really need to be addressed and solved. It's a larger issue than this PR, but let's start here by not breaking things that don't need to be.

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