kristianmandrup / schema-to-yup

Schema to Yup validation
Other
283 stars 50 forks source link

example is broken #97

Closed felixroos closed 2 years ago

felixroos commented 2 years ago

Hello! When testing the example on the README, the result is invalid. See this codesandbox.

Yups validate method throws the error age must be greater than undefined, which it is clearly not. When using the supposedly built yup schema directly (after "This would generate the following Yup validation schema:"), it works, so there clearly must be a bug.

kristianmandrup commented 2 years ago

Hmm, yeah. Have a look at the test cases. They all seem to pass. Could well be that the example in the Readme is outdated. I know a lot of other devs got it working. I've been refactoring and improving the library quite a bit lately.

kristianmandrup commented 2 years ago

I think I might know what the issue could be: exclusiveMinimum: 0, is an "edge case" where the 0 is falsy (ie same as undefined or null). I might have missed that.

kristianmandrup commented 2 years ago

The test case can be found in test/person-schema.test.js. Try the latest commit to master and add enable: {log: true} in the config object. Should provide some more insight now.

The key method is in ConstraintBuilder:

  presentConstraintValue(
    constraintValue,
    { constraintName, constraintFn, errFn }
  ) {
    if (!this.isPresent(constraintValue)) return;

    this.onConstraintAdded({ name: constraintName, value: constraintValue });

    if (this.isNoValueConstraint(constraintName)) {
      let specialNewBase = constraintFn(errFn);
      return specialNewBase;
    }

    // console.log("presentConstraintValue", { constraintName, constraintValue });
    const newBase = constraintFn(constraintValue, errFn);
    return newBase;
  }

onConstraintAdded({ name: constraintName, value: constraintValue }); should now call the same method on the YupBuilder instance which should log these values + the type name by default (if logging enabled) making it much easier to debug such issues in the future.

kristianmandrup commented 2 years ago

For some reason, when I run it locally I get:

schema-to-yup git:(master) npx jest person
 FAIL  test/person-schema.test.js
  ● Test suite failed to run

    ReferenceError: regeneratorRuntime is not defined

      1 | const { buildYup } = require("../src");
      2 |
    > 3 | test("converts person JSON schema to Yup Schema and validates", async () => {
        |     ^
      4 |   const personSchema = {
      5 |     $schema: "http://json-schema.org/draft-07/schema#",
      6 |     $id: "http://example.com/person.schema.json",

      at Object.<anonymous> (test/person-schema.test.js:3:5)
      at TestScheduler.scheduleTests (node_modules/@jest/core/build/TestScheduler.js:333:13)
      at runJest (node_modules/@jest/core/build/runJest.js:404:19)

Recently updated my Node version and various other Dev tooling and dependencies, VS code, extensions etc...

kristianmandrup commented 2 years ago

Solved the regeneratorRuntime issue by moving babel configuration from package.json to babel.config.js. Now debugging the issue.

kristianmandrup commented 2 years ago

Was exactly due to the constraintValue being evaluated as falsy when 0. Been fixed now. See latest release and updated docs. Person schema example works as expected now. Also added much better logging and logging control in the process. Cheers!

felixroos commented 2 years ago

wow that was a quick fix :) thank you.

kristianmandrup commented 2 years ago

Yeah, just goes to show how I've designed this to be very easy to customise and extend. Think of it more as a set of building blocks rather than a fully fledged "solution".