feat: preserveKeyOrder option

[P0lip]

Needed for stoplightio/spectral#730

[marbemac]

Is this a performance thing? Should we retain order of keys by default?

[P0lip]

Should we retain order of keys by default?

Not sure, probably not? I believe we want to stay close to JSON.parse, don't we?
Either way, this data won't be preserved anyway when the structured copy is performed, so well, Studio won't benefit too much regardless.

[marbemac]

Does JSON parse not retain key order? If this does not make a meaningful difference on performance, I suggest we do this by default.

[P0lip]

Does JSON parse not retain key order?

It does not.

If this does not make a meaningful difference on performance, I suggest we do this by default.

The parsing performance is not affected too much. It's around 25-35% slower, which isn't terrible.
The "trapped" access is slower by quite some marging in certain circumstances. I made a few Object.keys calls on a few properties from HugeYaml fixture, and they took 15-25 times longer.
Object.getOwnPropertyNames and Reflect.ownKeys aren't particularly slower - only 2-4 times. I didn't test Object.getOwnPropertySymbols, since it didn't make any sense.
That said, I guess we can make it disabled by default.

[marbemac]

Do you have a sense of how this sortKeys option affects performance?

[P0lip]

I'll check it.
Frankly speaking, though, I didn't expect to use it anywhere else than Spectral CLI, as in every other case the order does not really matter.

[marbemac]

Well... I would expect this to be true - xyz === yaml.stringify(yaml.parse(xyz)). Is this not the case right now?

[P0lip]

xyz === yaml.stringify(yaml.parse(xyz))

This will be true, but Object.keys(yaml.parse(xyz))[0] === Object.keys(yaml.parse(yaml.stringify(yaml.parse(xyz))))[0] won't if such a YAML document is provided

"foo": true
"0": false

Object.keys(xyz)[0] // equals "foo"
Object.keys(yaml.stringify(yaml.parse(xyz)))[0] // equals "0"

[marbemac]

xyz is a string right? So Object.keys(xyz) doesn't apply.

If key ordering is not preserved, I don't understand how xyz === yaml.stringify(yaml.parse(xyz)) could be true?

[P0lip]

Ah sorry, I meant to write Object.keys(yaml.parse(xyz))[0] // equals "foo" 😅

If key ordering is not preserved, I don't understand how xyz === yaml.stringify(yaml.parse(xyz)) could be true?

The key ordering should be preservered during stringification, but it won't be upon the second parsing unless preserveOrderKey is passed again.

I screwed the example there. Here is a correct one.

Object.keys(yaml.parse(xyz, { preserveKeyOrder: true }))[0] === Object.keys(yaml.parse(yaml.stringify(yaml.parse(xyz, { preserveKeyOrder: true }))))[0] // false

[P0lip]

You can also add a test if you'd like to

test('preserves the order of keys', () => {
  const xyz = `foo: true
bar: false
"1": false
"0": true
`;

  expect(safeStringify(parseWithPointers(xyz, { preserveKeyOrder: true }).data)).toEqual(
    safeStringify(parseWithPointers(safeStringify(parseWithPointers(xyz, { preserveKeyOrder: true }).data), {
      preserveKeyOrder: true,
    }).data as any),
  ); // this assertion is met

  expect(safeStringify(parseWithPointers(xyz, { preserveKeyOrder: true }).data)).toEqual(
      safeStringify(parseWithPointers(safeStringify(parseWithPointers(xyz, { preserveKeyOrder: true }).data)).data as any),
  ); // this assertion is not met
});

[marbemac]

Right but wouldn't one expect myString === JSON.stringify(JSON.parse(myString)) to be true?

Take Studio for example - if user opens YAML file, we parse it into memory and then stringify and write it back to disk. What it sounds like is that what is written back to disk will not equal what was originally on disk (key ordering not maintained).

[P0lip]

Take Studio for example - if user opens YAML file, we parse it into memory and then stringify and write it back to disk. What it sounds like is that what is written back to disk will not equal what was originally on disk (key ordering not maintained).

It depends on the value of preserveKeyOrder and the YAML document itself (if it has no numeric scalar mapping keys, everything will be okay).

[stoplight-bot]

🎉 This PR is included in version 3.4.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀