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

Optional field save behavior can break validations #241

Open
mshick opened this issue Oct 31, 2024 · 9 comments
Open

Optional field save behavior can break validations #241

mshick opened this issue Oct 31, 2024 · 9 comments
Assignees
Labels
enhancement New feature or request

Comments

@mshick
Copy link

mshick commented Oct 31, 2024

I see this listed as a feature under Better data output, but the save behavior for fields flagged as required: false can lead to data corruption and failed validations. In general it's pretty odd to populate values for data explicitly marked as optional / non-required.

An example would be a string field that on save is written as myfield: ''. If that field had a min length validation in a generator it would now fail to build. Likewise, empty string != undefined, and checks or logic built around presence or non-presence of a field would fail to behave as expected.

Ideally default values would only be set on fields that explicitly have a default value set in the configuration.

@kyoshino
Copy link
Member

kyoshino commented Oct 31, 2024

In my understanding, required: false (an optional field) means data input is not required, and doesn’t mean data output is not required. The String widget’s data type is a string, so an empty string is saved even if no value is entered. It’s expected/intentional behaviour (against the fuzzy behaviour of Netlify/Decap CMS) and I don’t plan to change that.

@kyoshino kyoshino closed this as not planned Won't fix, can't repro, duplicate, stale Oct 31, 2024
@kyoshino
Copy link
Member

kyoshino commented Oct 31, 2024

There are also data format limitations:

  • JSON and YAML don’t have undefined (there is null though)
  • TOML doesn’t have undefined nor null

@mshick
Copy link
Author

mshick commented Oct 31, 2024

YAML non-presence would effectively evaluate as undefined in JS. This is similar to JS itself:

cont obj = {}; 
console.log(typeof obj.field) // undefined

I'm not proposing Sveltia write the value undefined. As-is, you can't use Sveltia with something like Velite with the slug field, which enforces uniqueness and string length. Also, having a bunch of optional, unselected data dump into the md file on save is at the least very unexpected.

For instances, I have an optional object field cover.

{
  label: 'Cover',
  name: 'cover',
  widget: 'object',
  required: false,
  collapsed: true,
  fields: [
    {
      label: 'Image',
      name: 'image',
      widget: 'image',
      required: false
    },
    {
      label: 'Video',
      name: 'video',
      widget: 'string',
      required: false
    },
    {
      label: 'Title',
      name: 'title',
      widget: 'string',
      required: false
    },
    {
      label: 'Alt',
      name: 'alt',
      widget: 'string',
      required: false
    },
    {
      label: 'Caption',
      name: 'caption',
      widget: 'string',
      required: false
    }
  ]
}

If I save a file, even without this object "activated" via the UI switch, I end up with the following in my document, which makes it less readable, and it violates the presumed type contract here, since I have no way of modeling presence / non-presence of the cover object itself.

cover:
  image: ''
  video: ''
  title: ''
  alt: ''
  caption: ''

Basically, the behavior of writing defaults to every field in every case makes it impossible to model a type similar to:

type Document = {
  cover?: {
    image?: string
  }
}

At a minimum I'd say this defult-writing behavior should be optional, since it deviates substantially from Decap CMS.

@mshick
Copy link
Author

mshick commented Oct 31, 2024

Further, Decap / Sveltia supports setting a default. If I wanted an empty string output on an optional field I can set default: '' in the configuration.

Would you accept a PR to make the implicit-defaults-writing behavior optional? I like and am interested in this project, but this is sort of a non-starter.

@kyoshino
Copy link
Member

kyoshino commented Oct 31, 2024

An optional object is supposed to be saved as null by default. I will check why it’s not working.

Omitting an optional field key is not planned.

PRs are not accepted at this time.

@kyoshino
Copy link
Member

Also I want to clarify: Netlify/Decap CMS also saves an empty string on an optional string field in some cases.

@mshick
Copy link
Author

mshick commented Nov 3, 2024

A follow up, for anybody experiencing issues on this front, I was able to get the desired behavior (not writing empty strings for optional fields) with the following custom formatter code. It's working quite well.

    <script type="importmap">
      {
        "imports": {
          "yaml": "https://esm.sh/[email protected]"
        }
      }
    </script>
    <script type="module">
      import YAML from 'yaml'
    
      const optionalFields = ['my-field'] // get these somehow...

      const formatYAML = (content) => {
        for (const [k, c] of Object.entries(content)) {
          if (optionalFields.includes(k) && c === '') {
            delete content[k];
          }
        }

        return YAML.stringify(content, null, {
          lineWidth: 0,
          defaultKeyType: 'PLAIN',
          defaultStringType: 'PLAIN',
          singleQuote: true
        }).trim();
      }

      CMS.registerCustomFormat('yaml-frontmatter', 'md', {
        toFile({body, ...content}) {
          const delimiter = '---';
          body = body ? body + '\\n' : '';
          return delimiter + '\\n' + formatYAML(content) + '\\n' + delimiter + '\\n\\n' + body;
        }
      });
    </script>

@kyoshino
Copy link
Member

kyoshino commented Nov 3, 2024

I’d introduce a global option for changing the output behaviour. I’m fine as long as it’s consistent (unlike “sometimes omits, sometimes leaves” fuzziness in Netlify/Decap CMS). Stripping optional string type fields is easy — virtually a few-line change — but I need to check other field types.

output:
  omit_empty_optional_fields: true
  yaml:
    # YAML specific output options go here
    quote: single

I know Decap already has a PR for removing deleted image fields, but I’ll cover all the widgets.

@kyoshino kyoshino reopened this Nov 3, 2024
@kyoshino kyoshino added the enhancement New feature or request label Nov 3, 2024
@mshick
Copy link
Author

mshick commented Nov 4, 2024

Makes sense, and thanks for considering — I think that would be a nice enhancement. Definitely in favor of eliminating fuzz. In the case of my project, I'm working from zod types, and generating the CMS config, so stricter types are definitely preferable.

I'm unblocked with the method above, and I apologize, you were correct about the null for optional parents, that appears to be working as expected.

@kyoshino kyoshino self-assigned this Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants