fix: coerce undefined to string (#680)

[kyleaupton]

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• 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]

LGTM

[ivan-tymoshenko]

Hmm, this is a tricky one. When fjs receives an object with a property set to undefined, it skips the property. I guess, the logic here might be somehow similar. Although skipping array's items might not be the best idea.

What should fjs do if it receives an array with undefined property? Possible solutions I see:

• skip the item
• leave the item equal to the undefined
• throw an error (not a bad solution btw)

@mcollina wdyt?

@kyleaupton thanks for your PR, I will block it until we get a consistent answer on what to do in this case. If we decide to coerse it to the empty string it would require to also make some changes for all other types + a few use cases.

[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.

[Eomm]

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.

[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

[gurgunday]

So I agree that all functions need to conform to this behavior (nullish values get coerced to the falsy value of the corresponding type) if we're going to merge this:

fast-json-stringify/lib/serializer.js

Lines 53 to 62 in dfb627e

	asNumber (i) {
	const num = Number(i)
	if (Number.isNaN(num)) {
	throw new Error(`The value "${i}" cannot be converted to a number.`)
	} else if (!Number.isFinite(num)) {
	return 'null'
	} else {
	return '' + num
	}
	}