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

Question : why nullable and not type: ["null", ...] ? #162

Open
tonthon opened this issue Nov 22, 2023 · 7 comments
Open

Question : why nullable and not type: ["null", ...] ? #162

tonthon opened this issue Nov 22, 2023 · 7 comments

Comments

@tonthon
Copy link

tonthon commented Nov 22, 2023

First, thanks for the helpful work.

Following the Json schema specifications, nullable fields should include "null" in their type attribute

myfield: { "type": ["number", "null"] }

https://json-schema.org/understanding-json-schema/reference/null

Is there a specific reason why the nullable keyword is used to describe a nullable field in this library ?

@kristianmandrup
Copy link
Owner

You are absolutely right. It should be something like this: Fixing this now.

  nullable() {
    const nullable = this.value.nullable === true || this.value.null === true;
    this.base = (nullable && this.base.nullable()) || this.base;
    return this;
  }

I've also fixed nonNullable as per https://github.com/jquense/yup#schemanonnullable-schema which is set when null: false or nullable: false in the schema entry.

@kristianmandrup
Copy link
Owner

Please verify via the latest commit on master. Then I will publish new release

@hamsterhomka
Copy link

hamsterhomka commented Dec 13, 2023

Please verify via the latest commit on master. Then I will publish new release

I still can't see that array of types works

myfield: { "type": ["number", "null"] }

When I try to use it I get an error

Uncaught TypeError: Spread syntax requires ...iterable[Symbol.iterator] to be a function

@tonthon
Copy link
Author

tonthon commented Dec 13, 2023

@kristianmandrup

Sorry for the last answer but it seems you've added the ability to put

"age": {
      "description": "Age of person",
      "type": "integer",
      "moreThan": 0,
      "max": 130,
      "default": 32,
      "required": false,
      "null": true
    }

but it's still not jsonschema compliant

"age": {
      "description": "Age of person",
      "type": ["integer", "null"],
      "moreThan": 0,
      "max": 130,
      "default": 32,
      "required": false,

    }

@kristianmandrup
Copy link
Owner

Correct. This is due to MultiTypeResolver handling any type as string as a potential schema type with coresponding type handler. Null is currently not a type, but is handled by the Mixed type handler.

See normalizedValue below from multi-property-value-resolver.js#L22-L45 where a string value in a type array is normalized to an object of the form {type: type} such as string becoming {type: "string"} and then handled.

  oneOf() {
    const schemaValues = this.value
    const createEntry = this.createEntry.bind(this)
    const resolvedValidatorSchemas = schemaValues.map(createEntry)
    return this.mixed().oneOf(resolvedValidatorSchemas)
  }

  notOneOf() {
    const schemaValues = this.value
    const createEntry = this.createEntry.bind(this)
    const resolvedValidatorSchemas = schemaValues.map(createEntry)
    return this.mixed().notOneOf(resolvedValidatorSchemas)
  }

  createEntry(value) {
    const { createYupSchemaEntry } = this.config
    value = normalizedValue(value)
    const opts = { schema: this.schema, key: this.key, value, config: this.config }
    return createYupSchemaEntry(opts)
  }

  normalizedValue(value) {
    return typeof value === 'string' ? {type: value}: value
  }

A workaround would be to add an additional type handler for null as described in the Advanced configuration documentation.

Which would be sth like YupNull

class YupNull extends YupMixed {
  // leverage existing YupMixed handling of null
}

I think the above could work without any meat inside YupNull. If so, you could even add the additional type handler and point it to YupMixed directly

@kristianmandrup
Copy link
Owner

@caioedut
Copy link

@kristianmandrup any news?

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

No branches or pull requests

4 participants