Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserving Map and Set types, and handling Complex Keys #78

Closed
isaacs opened this issue Jan 26, 2019 · 4 comments
Closed

Preserving Map and Set types, and handling Complex Keys #78

isaacs opened this issue Jan 26, 2019 · 4 comments
Labels
enhancement New feature or request

Comments

@isaacs
Copy link
Contributor

isaacs commented Jan 26, 2019

Maps with complex keys are stringified properly, as one would expect, with ? key: ...\n: value. However, when parsed, the key is converted to a JSON string.

const yaml = require('yaml')
const a = Symbol('asdf')
const b = Symbol('basd')
const data = {
  cmap: new Map([[ { complex: 'key', a: [1,2,3] }, 7 ]]),
}
console.log('js data:\n---\n', data)
console.log('...')
const yamlData = yaml.stringify(data)
console.log('yamlData:\n---\n%s...', yamlData)
const loaded = yaml.parse(yamlData)
console.log('loaded\n---\n', loaded)
console.log('...')
const dumped = yaml.stringify(loaded)
console.log('dumped:\n---\n%s...', dumped)

output:

js data:
---
 { cmap: Map { { complex: 'key', a: [Array] } => 7 } }
...
yamlData:
---
cmap:
  ? complex: key
    a:
      - 1
      - 2
      - 3
  : 7
...
loaded
---
 { cmap: { '{"complex":"key","a":[1,2,3]}': 7 } }
...
dumped:
---
cmap:
  "{\"complex\":\"key\",\"a\":[1,2,3]}": 7
...

I realize that I can use the mapAsMap option to always use Map objects instead of plain old javascript objects, but this isn't ideal, since it converts plain JavaScript objects to Map objects. As a result, either we always convert Map objects to POJOs, or always convert POJOs to Maps. I'd like a way to deterministically return a Map for a Map, and a POJO for a POJO.

Map Handling Proposal:

  • Map objects always stringify with an explicit (complex) key. So an object like new Map([['x', 'y']]) would stringify to
    ? x
    : y
  • If every key is explicit, or any key is complex, use a Map instead of a JavaScript object.
  • If desired, maybe add a preserveMapType option to use this behavior.

Similarly with Set objects, a set like new Set([1, 2, 3, 3]) will end up being converted to the JavaScript array [1, 2, 3]. However, a Set is more closely aligned with the yaml Set type:

set:
  ? 1
  ? 2
  ? 3

or in flow lingo: {1, 2, 3}.

So, a similar proposal for handling Set objects could be:

  • Set objects always stringify to a yaml Set.
  • If every key in a set has an explicit ? key and no value, return a Set object rather than an array.
  • If desired, add a preserveSetType option to use this behavior.

The rule of thumb I'd like to get to is that, for as many objects as possible: yaml.stringify(object) === yaml.stringify(yaml.parse(yaml.stringify(object))).

I'm happy to dig into the code to prepare a PR if this seems like an acceptable design. Thanks for your consideration.

@eemeli
Copy link
Owner

eemeli commented Jan 26, 2019

Supporting idempotence on JS/YAML transformations does seem like a pretty good goal. So far I've mainly focused on making sure that YAML -> Document -> YAML transformations stay as constant as possible.

Fundamentally, the most difficult part to handle here is that the default Core Schema of YAML 1.2 only supports the !!seq and !!map collections, and on the JS side we have all of Array, Object, Set and Map. However, version 1.1 of the spec does include other collection types, and those do have built-in support in yaml. For Set there's !!set, and for Map we have !!omap (as the Map items are indeed ordered, and with the default false value for mapAsMap the implicit !!map corresponds to Object). To use them while keeping the rest of the 1.2 semantics, you can extend the default schema with those particular collection types:

import YAML from 'yaml'
import omap from 'yaml/types/omap'
import set from 'yaml/types/set'

YAML.defaultOptions.tags = [omap, set]

YAML.stringify(new Map([[42, 'x']])) // '!!omap\n- 42: x\n'
YAML.parse(YAML.stringify(new Map([[42, 'x']]))) // Map { 42 => 'x' }

YAML.stringify(new Set([true, false])) // '!!set\n? true\n? false\n'
YAML.parse(YAML.stringify(new Set([true, false]))) // Set { true, false }

Besides the above, there's the question of how to represent in JavaScript maps with non-string keys and maps with all-null values when using the default schema, and for that case I think you're right, the mapAsMap option is not enough, and should be superseded by a choice between pojo/auto/es6 (better names are welcome). The auto level might need to be split into two, as it's not always obvious how a user might prefer something like 42: x or true: false to be represented -- would they want those keys to be strings or integer/boolean values?

I do not think that considering the string representation style of the YAML key is by itself sufficient, as e.g. [foo]: bar is an entirely valid representation of a pair with a key equivalent to [ "foo" ]. It's conceivable that a map having ? complex keys could be taken into account by the heuristics deciding between Object and Map, but my gut feeling would be to prefer not doing that.

Does that about cover the cases that you had in mind, or did I miss some part?

@isaacs
Copy link
Contributor Author

isaacs commented Jan 26, 2019

I think that does cover it fairly well, and explicitly using omap and set types seem like a way I could go! I don't have to parse arbitrary yaml in most of tap's cases, just the yaml that tap itself produces, but it has to be able to produce intelligible yaml out of any random garbage that a js test file might produce.

I'm not getting a Set out of yaml.parse(yaml.stringify({ set: new Set([1,2]) })) though, only if it's the root object. Here's an example, maybe I'm doing something wrong? https://gist.github.com/isaacs/d6843c4d72da09bf786a6c16f7948126

@eemeli
Copy link
Owner

eemeli commented Jan 26, 2019

Nope, that's a bug. This is wrong:

const good = YAML.stringify(new Set([1, 2])) // '!!set\n? 1\n? 2\n'
const bad = YAML.stringify({ set: new Set([1, 2]) }) // 'set:\n  ? 1\n  ? 2\n'

The bad value should also tag the collection as a !!set.

@eemeli eemeli added bug Something isn't working enhancement New feature or request labels Jan 26, 2019
eemeli added a commit that referenced this issue Jan 27, 2019
@eemeli eemeli removed the bug Something isn't working label Jan 27, 2019
@isaacs
Copy link
Contributor Author

isaacs commented Jan 28, 2019

Thanks for fixing. I think with that in place, I have clear way to ensure that Maps turn into Maps and Sets turn into Sets, in both directions. I'll close this issue, I agree that the implicit handling is probably too spooky.

@isaacs isaacs closed this as completed Jan 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants