Skip to content

Configurable fields #132

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.

Sign up for GitHub

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

Jump to bottom
Closed
psrpinto wants to merge 27 commits into from
Closed

Configurable fields #132

psrpinto wants to merge 27 commits into from

Conversation

@psrpinto
Copy link
Member

@psrpinto psrpinto commented Dec 2, 2024 •
edited
Loading

This PR introduces no user-visible changes. From the user's perspective, everything still works as it did before.

This PR implements the solution described in #101, so that the fields that are available for a given subject (blog post, page, etc), are provided through JSON configuration. Each subject type has its own configuration, which I've named a schema.

Note that I opted for splitting the schema of each subject into it's own file. I think that having all of them in the same file would not scale too well, the file would grow to infinity and become difficult to maintain. So, instead, each subject has its own schema:

// ./src/schemas/blog-post.json
{
    "title": "Blog Post",
    "fields": {
        "title": {
            "type": "text"
        },
        "date": {
            "type": "date"
        },
        "content": {
            "type": "html"
        }
    }
}

Additionally, JavaScript and PHP functions are provided to load the schema of a given subject type:

// JS
export function getSchema( subjectType: SubjectType ): object
// PHP
function get_schema( string $subject_type ): array|null

Frontend

The frontend code has been fully adapted to use the schemas, which means that the code is now generic. All the specific types (BlogPost, Page, BlogPostBlueprint, etc) have been removed, and the only types left are generic (Subject, Blueprint. This is how the code should have been from day one, we're now reaching the "correct" solution.

This results in the code being massively simplified, and adding a new subject type does no longer require changing a bunch of files, and copy-pasting code around.

A new SubjectsApi has also been introduced, and the specific BlogPostsApi and PagesApi were removed. However, internally, SubjectsApi, still differentiates between subject types because it needs to call different endpoints depending on the subject type. Once we tackle the backend side of this (see below), I expected that to no longer be necessary.

Backend

This PR does not make any changes to the backend. Instead, as mentioned above, it introduces a generic SubjectsApi that abstracts the fact that the backend hasn't been changed. Once we tackle the backend side of this, I think it would make sense that the backend exposes a generic /subjects API (as opposed to specific /blog-posts or /pages). At that point, the SubjectsApi can become truly generic.

I took the liberty of opening #133 to track that work.

Screen capture

Screen.Recording.2024-12-02.at.18.16.53.mov

@psrpinto psrpinto self-assigned this Dec 2, 2024
@psrpinto psrpinto force-pushed the schemas branch 4 times, most recently from 0c7240f to 77f5070 Compare December 2, 2024 16:04
@psrpinto psrpinto marked this pull request as ready for review December 3, 2024 15:45
@psrpinto psrpinto requested review from akirk and ashfame December 3, 2024 15:45
@psrpinto psrpinto mentioned this pull request Dec 3, 2024
Closed
@akirk
Copy link
Member

akirk commented Dec 5, 2024

Note that I opted for splitting the schema of each subject into it's own file. I think that having all of them in the same file would not scale too well, the file would grow to infinity and become difficult to maintain.

I disagree with this and see it as premature optimization which complicates things. It should be a single JSON file that can be looped over. The content types are inferred from the keys.

In a format where we have multiple files we need an index that points to other files, but what we need is the flexibility to add new fields and types.

I am not opposed to moving forward with the PR as is but I really believe we need to have the flexibility to add more content types by just adding new JSON entries, without adding new TypeScript code.

@akirk
Copy link
Member

akirk commented Dec 5, 2024

Just to clarify a little (and also backtracking a bit): I am not opposed to having multiple files but adding new content types should be so generic that no code needs to be changed. I tried adding a product as a test and it needs changes in many places in code.

@psrpinto
Copy link
Member Author

psrpinto commented Dec 5, 2024

I disagree with this and see it as premature optimization which complicates things.

I don't agree that it is premature optimization, but even if it would be, if there would have been more premature optimization in WordPress, it wouldn't be in the state that it is today. It's my responsibility as an engineer who is concerned with the maintenance burden of software I produce, to predict hot spots of maintenance burden and "prematurely" prevent them. I consider a file that will grow to infinity to be such a hot spot. There's a difference between premature optimization and solid engineering practices.

In addition, since I predicted this would be a contentious point, I took steps to minimize the impact of splitting the schema into multiple files, by providing both JavaScript and PHP functions that abstract the fact that those schemas are stored in separate files. The schemas should always be consumed through those functions, and such functions would need to exist anyway even if schemas would be stored in a single file. The fact that schemas are stored in separate files is transparent to code that needs to consume them.

we need to have the flexibility to add more content types by just adding new JSON entries, without adding new TypeScript code.

Yes, this is the goal. However, as I mention in the PR description, for this to be possible, I believe the backend must also provide a generic API. Currently, the boundary of where genericity is broken is in SubjectsApi, so that file must be changed if we want to add a new subject. For that not to be the case, there must be a generic /subjects API that takes any type of subject. I opened #133 to track that.

@psrpinto psrpinto mentioned this pull request Dec 5, 2024
Closed
@psrpinto
Copy link
Member Author

psrpinto commented Dec 5, 2024

I will be splitting this PR in two, since it's currently doing two things and I would like those things to be addressed in separate PRs:

  1. Define the structure of the schema and how to load it into javascript and PHP
  2. Make the frontend use the schema

@psrpinto psrpinto closed this Dec 5, 2024
@psrpinto psrpinto mentioned this pull request Dec 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ashfame ashfame Awaiting requested review from ashfame

@akirk akirk Awaiting requested review from akirk

Assignees

@psrpinto psrpinto

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@psrpinto @akirk