fix: coerce undefined to string (#680)

[kyleaupton]

Checklist

• [ ] run npm run test and npm run benchmark
• [x] tests and/or benchmarks are included
• [ ] documentation is changed or added
• [x] commit message and code follows the Developer's Certification of Origin and the Code of conduct

On my end all test pass, however coverage is at 96% which is causing an error.

Suites:   49 passed, 49 of 49 completed
Asserts:  818 passed, of 818
Time:   4s
ERROR: Coverage for functions (96%) does not meet global threshold (100%)
-------------------------|---------|----------|---------|---------|-------------------
File                     | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
-------------------------|---------|----------|---------|---------|-------------------
All files                |   98.54 |    98.16 |      96 |   98.69 |
 fast-json-stringify     |   99.75 |    98.21 |     100 |   99.75 |
  index.js               |   99.75 |    98.21 |     100 |   99.75 | 508
 fast-json-stringify/lib |   95.03 |    98.05 |    91.3 |   95.58 |
  location.js            |     100 |      100 |     100 |     100 |
  merge-schemas.js       |     100 |      100 |     100 |     100 |
  serializer.js          |   98.85 |      100 |    90.9 |   98.79 | 165
  standalone.js          |     100 |      100 |     100 |     100 |
  validator.js           |   83.33 |     91.3 |   85.71 |   85.71 | 36,86-90
-------------------------|---------|----------|---------|---------|-------------------

[Uzlopak]

Feels wrong to skip Elements.

[ivan-tymoshenko]

Yeah, this is not an easy one.

[mcollina]

@ivan-tymoshenko this issue is about strings, casting a undefined to an empty string seems ok to me. Dropping the value seems not good, or keeping it undefined.

If we pass an number to a type string, it will be casted to a string. Why undefined should be different?

[gurgunday]

I like this change for the same reasons as @mcollina

[ivan-tymoshenko]

If we pass an number to a type string, it will be casted to a string. Why undefined should be different?

Because if you have an object schema with a string property and pass an object with property equal to undefined it will skip the property instead of coercing it to the empty string.

[ivan-tymoshenko]

this issue is about strings, casting a undefined to an empty string

This issue can't be only about strings anyway. It should be about all types.

[Uzlopak]

Countering your argument JSON.stringify({ a: undefined}) // {} vs. JSON.stringify([undefined]) // [null]

[ivan-tymoshenko]

Since when did we start use JSON.stringify as a rule to go?

[Uzlopak]

I use it now as an argument.

[ivan-tymoshenko]

Since we are casting null to "", it makes sense to do the same for undefined.

In the context of JSON.stringify, both null and undefined result in null, so maintaining consistency in our handling makes sense, so both serialize as string.

If you want to cast the undefined to the empty string if it's not an object property and skip it if it's an object property, then it should also coerce every other type from undefined to it's nullish value.

[gurgunday]

Do you mean for instance that in the same scenario but for a number, it should get coerced to 0?

I would agree that it’s what this tool ought to behave like