Use the null prototype when parsing YAML maps#678
Use the null prototype when parsing YAML maps#678remcohaszing wants to merge 3 commits intoeemeli:mainfrom
Conversation
YAML maps map a key to a value. These keys may conflict with keys on the JavaScript `Object` prototype. Several JavaScript APIs that use a plain object to represent a key/value map, use a null prototype. Closes eemeli#675
|
I realized |
| ctx: ToJSContext | ||
| ): ReturnType<typeof addPairToJSMap> { | ||
| const pair = ctx.mapAsMap ? new Map() : {} | ||
| const pair = ctx.mapAsMap ? new Map() : { __proto__: null } |
There was a problem hiding this comment.
Directly setting __proto__ is deprecated, and not the way we should do this. Please use Object.create(null) instead.
There was a problem hiding this comment.
Actually, I see that article states:
The
__proto__property can also be used in an object literal definition to set the object[[Prototype]]on creation, as an alternative toObject.create(). See: object initializer / literal syntax. That syntax is standard and optimized for in implementations, and quite different fromObject.prototype.__proto__.
And the linked Object initializer page even uses it in examples.
So I think my initial implementation was good, and slightly more readable.
The use of `__proto__` is deprecated.
eemeli
left a comment
eemeli
left a comment
There was a problem hiding this comment.
There's a linter failure, otherwise looks good!
|
The documentation will also need an update, at least under Document#toJS(). |
|
A friend of mine did some benchmarking on null prototypes and figured that objects with a null prototype provide a 3x memory penalty and a significant access penalty vs regular objects, because of V8 optimizations. So maybe it’s better to not merge this PR altogether. |
2 participants
YAML maps map a key to a value. These keys may conflict with keys on the JavaScript
Objectprototype. Several JavaScript APIs that use a plain object to represent a key/value map, use a null prototype.Closes #675