-
-
Notifications
You must be signed in to change notification settings - Fork 776
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
Trying to use an object as a key results in the string '[object Object]' as a key #169
Comments
I guess, it should throw, because JSON is invalid. |
It is valid. It is not, obviously. YAML allows to use anything as a mapping key. However, JS-YAML loads YAML mappings into JS objects, and we have to convert YAML key objects into strings. This will be fixed in future with Map support. |
The YAML being valid doesn't mean this should be closed, surely? Even if it's valid YAML, the behaviour here isn't useful; throwing an error indicating that this is valid YAML but |
There are other options too, like parsing it into some structure other than an Object or taking an argument that indicates whether to throw on object keys or not. But silently turning object keys into |
Sorry. I closed this issue because that behavior of JS-YAML is really old and even described in the README. However, you have a good point, and it's time to fix the problem. We've just discussed this internally, and here is the plan:
|
I doubt this problem can be solved easy, because maps have completely different interface, and it will be very hard to make both (objects + map) support AND fast implementation. May be we could add maps support for explicits, but i should be sure, that it's really needed. I'd suggest this behaviour:
That's enougth to show clear that shit happened. |
Yeah, I'm not too familiar with ES6 maps but given the difference in interface it seems that using objects where possible and maps only where non-string keys are needed would be pretty unhelpful and confusing behaviour. And swapping to maps altogether, besides being a significant breaking change, is current inviable for anybody who needs to support environments that don't have maps. I'd guess that the ideal thing for you to do would be:
|
You missedt the main question - why maps support should be added at all :) . If nobody use it - no reasons to spend time for adding support. |
Something else relevant: using an array as a key is apparently legal YAML, but results in a string representation of the array being used as the key.
that's ultimately a call for you guys, and you're probably better qualified to make it than me, but let me toss out my thoughts for whatever it's worth.
I can understand classifying map support as low priority, and it might make sense to add the warning without the Map support for now, but it seems to me like it would be very strange to actively decide against adding it. |
I have no motivation to spend resources for adding native maps support. If someone wish to do it - PR will be accepted. AFAIK, even in current state js-yaml is significantly better than alternative implementations. So, i'd prefer to focus on another projects. |
I just ran into this. I am trying to use YAML as a kind of language-independent graph serialization protocol. I have a graph of arbitrary Java objects on a server that I want to display over the web. Those server-side objects include Maps that use Objects as keys. |
For what it's worth, I took at stab at replacing use of Object with Map in the loader. I'm not sure that I got it completely correct, I did not measure performance, I did not worry about compatibility, I did not make it an opt-in change, etc., but the primary work took less than an hour. It got me past the duplicate key problem resulting from use of String(object), but then I hit an aliasing problem and I don't know whether it was related. Ultimately I don't think I'll be continuing with YAML. |
I also ran on this issue here. I thought about allowing users to represent a request object with key/value pairs that could be either function or scalars like: ? !someTypeBecomingAJsFunction someData : someValue
? someKey : !someTypeBecomingAJsFunction someData I read the YAML specification and was ecstatic to find out it supported my strange use case out of the box. After running the YAML parser I could easily transform this into a function that if called with arguments produces an object representing the intended structure. But I also agree that the default object behavior is better for ease of use (the Map API is sadly not accepting |
This sounds like the best approach. |
...and without significant speed loss for legacy mode. |
I created a little script to compare how different JavaScript based YAML parser handle this issue: https://gist.github.com/adius/0bb08111c16a15fb34c68b5a24b817d0 |
If I throw the following markup (which I believe is not valid YAML, though I'm not even remotely an expert) into the online YAML parser at http://yaml-online-parser.appspot.com/ ...
{{"foo": "bar"}: {"qux": "baz"}}
... then I get:
If I throw it into
yaml.safeLoad
, on the other hand, I don't get an error. Instead I get this:Whatever the correct behaviour here is (and I defer that question to those more knowledgable than me), this is plainly not it.
The text was updated successfully, but these errors were encountered: