o2k: reorder object members to prettify the yaml output

[Tieske]

The o2k output has the object members in random order, depending on where they were generated. This adds a reordering that prettifies the final yaml output to be more readable.

fixes INS-671

[Tieske]

wrt failing CI; tried a bunch of things but couldn't get past Flow. Generation all works when testing manually.

[Tieske]

k, managed to fix the linter issues. all green now.

[develohpanda]

Lookin' good to me! We'll come back to merge this after code-freeze for TS lifts 🙏🏽

[dimitropoulos]

(I updated to TypeScript for you)

[dimitropoulos]

I just want to drop here that I fundamentally do not approve of doing something like this. Objects are explicitly unordered in JavaScript, so we need to find another way to accomplish this.

An object is a member of the type Object. It is an unordered collection of properties each of which contains a primitive value, object, or function.

• https://www.ecma-international.org/wp-content/uploads/ECMA-262_3rd_edition_december_1999.pdf

Furthermore, it's also explicitly unordered in YAML:

The content of a mapping node is an unordered set of key: value node pairs, with the restriction that each of the keys is unique.

• https://yaml.org/spec/1.2/spec.html

While some JavaScript engines respect insertion order, almost by accident of implementation, others do not (for example, the engine used in safari). I've encountered bugs in real-world applications in the past due to changes precisely like this.

But at the same time, I can definitely see the value of something like this for creating deterministic output files. I think, even in that case, we should explore using something like https://www.npmjs.com/package/json-stable-stringify in place of our JSON.stringify calls. That way, we don't have to depend on developers remembering to sort at every which-place, and it greatly reduces room for future error.

I'm happy to help with this, lemme know.

[dimitropoulos]

CORRECTION:

As of ECMAScript 2020 this has been standardized https://github.com/tc39/proposal-for-in-order

I thought maybe this would throw it off but it doesn't:

All the same, I can't find this exact proposal listed anywhere or in v8 release notes but https://kangax.github.io/compat-table/es2016plus/ is the closest thing I can find, and judging by this it will be at least until Node 16 (note, we're on Node 10 now) that we can consider this spec applicable to our codebase. That means the above holds, although only for another few months or years, thankfully. On the other hand, v8 already worked this way (it was JavaScript Core, used in Safari, that didn't) so maybe it's fine. I donno.

The other thing is, this is hard-ish to test:

  it("demonstrates that order doesn't matter in tests", () => {
    const x = {
      a: 1, // `a` comes first
      b: 2,
    };
    const y = {
      b: 2,
      a: 1, // `a` comes second
    };
    expect(x).toEqual(y); // this passes
    expect(x).toStrictEqual(y); // this passes
  });

We would, after all, really want some tests for this if we are going to start caring about it and this PR introduces no tests. I'm happy to help with this. I think with the stable stringify approach we can make it happen and I'd be happy to contribute some tests or fix all existing tests (we'd likely want/have to many many of them).

[develohpanda]

Looks like YAML stringify has an option to sort keys

[Tieske]

The goal here was not to sort, but make it human friendly, hence the PR only defines order for the first entries, and the last entries. Using a sorted output (basically canonicalization) doesn't make sense to me. No parser should ever rely on order. Any follow steps in a ci/cd pipeline will most likely mess it up again.

So the question is; is there a use case for canonicalized output? (my 2cts; no)

if yes; then we skip human friendly output
if no, then we can fallback to human friendly output

[dimitropoulos]

@develohpanda as an aside for later, re yaml:

We needed to upgrade to at least 1.8 to get the sortMapEntries and to 1.9 to get native types (at all). In 2.0 they rewrote it in native typescript, so I went for that one. I was sure to check all 16 places in the monorepo where we import yaml to verify that there are no breaking changes that affect us, and, indeed, we only use strignfiy and parse, so we're in the clear.... except for just one place. In https://github.com/Kong/insomnia/blob/develop/packages/insomnia-app/app/ui/components/spec-editor/spec-editor-sidebar.tsx#L70 we call parseDocument and pass an option keepCstNodes which was removed in 2.0 (eemeli/yaml#235). I looked into it, and it is non-trivial (and out of scope for this PR) to make this change, I think. That means until we update this one usage we can't upgrade yaml, which, is a shame because we currently are asking for 4 different versions across 7 usages:

[wongstein]

@Tieske Can I close this one out?