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: @amplienceContentType fieldOrder #101

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jonnoallcock
Copy link
Contributor

@jonnoallcock jonnoallcock commented Jun 18, 2024

New optional fieldOrder argument for @amplienceContentType. Allows the developer to explicitly control the propertyOrder in a given content type's JSON output.

Example:

The following schema:

type OrderedFields @amplienceContentType(fieldOrder: "c b a e d") {
  a: String!
  b: String!
  c: String!
  d: String!
  e: String!

  ignored: String @amplienceIgnore
  deliveryKey: String @amplienceDeliveryKey
}

Outputs:

{
  "$id": "https://schema-examples.com/ordered-fields",
  "$schema": "http://json-schema.org/draft-07/schema#",
  "allOf": [
    { "$ref": "http://bigcontent.io/cms/schema/v1/core#/definitions/content" }
  ],
  "title": "Ordered Fields",
  "description": "Ordered Fields",
  "type": "object",
  "properties": {
    "_meta": {
      "type": "object",
      "title": "Delivery Key",
      "properties": {
        "deliveryKey": {
          "type": "string",
          "title": "Delivery Key",
          "description": "Set a delivery key for this content item"
        }
      }
    },
    "a": { "title": "A", "type": "string" },
    "b": { "title": "B", "type": "string" },
    "c": { "title": "C", "type": "string" },
    "d": { "title": "D", "type": "string" },
    "e": { "title": "E", "type": "string" }
  },
  "propertyOrder": ["_meta", "c", "b", "a", "e", "d"], // explicit propertyOrder
  "required": ["a", "b", "c", "d", "e"]
}

Validation

Must not specify fields with @amplienceDeliveryKey or @amplienceIgnore
(These are handled uniquely with regards to JSON output, so do not need specifying)

type Test @amplienceContentType(fieldOrder: "ignored a deliveryKey") {
  a: String
  ignored: @amplienceIgnore
  deliveryKey: @amplienceDeliveryKey
}

# Error:
# @amplienceContentType fieldOrder arguments must not specify fields with @amplienceDeliveryKey or @amplienceIgnore.
#   type Test
#     ignored
#     deliveryKey

Must specify all extant non-illegal fields

type Test @amplienceContentType(fieldOrder: "b a c") {
  a: String
  b: String
  c: String
  d: String
  e: String
}

# Error:
# @amplienceContentType fieldOrder arguments must specify every field defined within the type.
#   type Test
#     d
#     e

Must not specify unknown field names

type Test @amplienceContentType(fieldOrder: "unknownOne a b unknownTwo") {
  a: String
  b: String
}

# Error:
# @amplienceContentType fieldOrder arguments must not specify fields not defined within the type.
#   type Test
#     unknownOne
#     unknownTwo

@jonnoallcock jonnoallcock force-pushed the feat/amplience-content-type-field-order branch 3 times, most recently from 67d2a3f to b081534 Compare June 18, 2024 15:46
@jonnoallcock jonnoallcock changed the title Feat/amplience content type field order Feat: @amplienceContentType fieldOrder Jun 18, 2024
@jonnoallcock jonnoallcock changed the title Feat: @amplienceContentType fieldOrder feat: @amplienceContentType fieldOrder Jun 18, 2024
@korsvanloon
Copy link
Collaborator

Can you run pnpm changeset? This should generate a file and please add that to this PR.
(I'll add this instruction in the README later ;) )

@korsvanloon
Copy link
Collaborator

The current behavior is to keep the GraphQL definition order. Can you tell me why this isn't sufficient? What problem is this PR trying to solve?

@jonnoallcock
Copy link
Contributor Author

The current behavior is to keep the GraphQL definition order. Can you tell me why this isn't sufficient? What problem is this PR trying to solve?

Current behavior is actually to output fields in alphabetical order (see content-type.json). This results in some pretty counterintuitive content forms for our editors, so we wanted control over the output to improve that experience.

I was hoping for an automatic solution to respect definition order, but found that:

  • the plugin receives fields already sorted
  • the plugin does not receive any information pertaining to the original order and cannot infer it with the information it does have

This explicit argument felt a good compromise.

@korsvanloon
Copy link
Collaborator

Have you tried to disable sorting in the codegen file? Do you run into any other problems with that?

import type { CodegenConfig } from "@graphql-codegen/cli";

const config: CodegenConfig = {
  overwrite: true,
  config: {
    sort: false,
  },
  generates: {
   ...

@jonnoallcock
Copy link
Contributor Author

Have you tried to disable sorting in the codegen file? Do you run into any other problems with that?

import type { CodegenConfig } from "@graphql-codegen/cli";

const config: CodegenConfig = {
  overwrite: true,
  config: {
    sort: false,
  },
  generates: {
   ...

...I did try that as a first attempt during investigation well before looking at a plugin solution, and found it didn't appear to work for whatever reason. Trying it now...it does. Suffice it to say I'm more than puzzled.

Where does that leave us with this feature? Reckon it's worth keeping for any potential use case, or dropping outright?

@korsvanloon
Copy link
Collaborator

I think it's because somehow the sort only works in the root config, and not in a config scoped within a generate.

I'll keep this PR open for a couple of weeks to be sure.
You might run into other issues, then we can merge this. If not I'll close this PR after a couple of weeks.

@jonnoallcock jonnoallcock force-pushed the feat/amplience-content-type-field-order branch from 710e181 to 6ccbaab Compare June 19, 2024 08: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.

2 participants