fix(yaml): fix Boolean object handling

[timreichen]

Closes #5857

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.35%. Comparing base (f86e911) to head (902aaad).
Report is 8 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5876   +/-   ##
=======================================
  Coverage   96.34%   96.35%           
=======================================
  Files         479      479           
  Lines       38674    38673    -1     
  Branches     5631     5630    -1     
=======================================
+ Hits        37260    37262    +2     
+ Misses       1370     1367    -3     
  Partials       44       44           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[iuioiua]

@kt3k, PTAL at the related issue. Do you agree this is a fix? I think it is as it fixes expected behavior.

[kt3k]

This changes the result of stringify({ foo: new Boolean(true) }) to foo: {} instead of foo: true. I don't think this matches the expectation of the users.

I think we should fix represent handler of bool type instead. I guess something like Boolean(object) ? "true" : "false" should fix the issue.

[timreichen]

This changes the result of stringify({ foo: new Boolean(true) }) to foo: {} instead of foo: true. I don't think this matches the expectation of the users.

I think we should fix represent handler of bool type instead. I guess something like Boolean(object) ? "true" : "false" should fix the issue.

The problem is that this special treats the Boolean object instance. We have the same problem with

stringify({ foo: new String("foo") })

which returns

foo:
  '0': f
  '1': o
  '2': o

and

stringify({ foo: new Number(1) })

which returns

foo: {}

[timreichen]

Tbh, I don't know if we should special treat these cases as they should be discouraged anyway.

[kt3k]

The problem is that this special treats the Boolean object instance. We have the same problem with

stringify({ foo: new String("foo") })

which returns

foo:
  '0': f
  '1': o
  '2': o

and

stringify({ foo: new Number(1) })

which returns

foo: {}

Those examples also seem bugs to me.

std/yaml is originally ported from js-yaml and js-yaml handles those cases as foo: foo and foo: 1.

import * as yaml from "npm:js-yaml";

console.log(yaml.dump({ foo: new String("foo") }));
console.log(yaml.dump({ foo: new Number(1) }));

[kt3k]

Looks like those handlings of new String and new Number are regression in recent versions. It used to be handled as string and number:

import { stringify } from "https://deno.land/[email protected]/yaml/mod.ts";

console.log(stringify({ foo: new String("foo") }));
console.log(stringify({ foo: new Number(1) }));

This prints:

foo: foo
foo: 1

[timreichen]

Closing in favour of #5894