LongTengDao / j-toml

A Node.js implementation of TOML written by LongTengDao. Belong to "Plan J"./龙腾道为汤小明语写的 Node.js 实现。从属于“简计划”。
https://npmjs.com/package/@ltd/j-toml
GNU Lesser General Public License v3.0
55 stars 6 forks source link

NULL prototype thing causes trouble #24

Open chris-morgan opened 2 years ago

chris-morgan commented 2 years ago

The parse method produces objects with a strange prototype. Rather than leaving it at Object.prototype (the default) or using null (mostly better), the objects get an indirect prototype, an object that itself has a null prototype, and is also sealed. I cannot imagine any valid reason for this: it’s purely inefficiency. But it also thwarts detection of a null prototype, which is a bad thing, because “is its prototype Object.prototype or null” is a customary “simple object” check. For a concrete example of what this breaks, trying to use require("@ltd/j-toml").parse in Eleventy data or front matter ends up ignoring everything if deep data merging is enabled (which it is by default), because it does this kind of “simple object” check, or uses the iteration prototcol if it isn’t simple—and this gets erroneously detected as not simple.

I propose removing this:

const NULL = (
    /* j-globals: null.prototype (internal) */
    Object.seal
        ? /*#__PURE__*/Object.preventExtensions(Object.create(null))
        : null
    /* j-globals: null.prototype (internal) */
);

And replacing all occurrences of NULL with null. Simpler, faster, more efficient, less troublesome. There may be more related stuff to it, but I’ve decided not to use this library because of this and other troublesome prototype shenanigans (e.g. how Datetime effectively deletes methods, which is a bad thing to do and almost always worse than just not doing that—yeah, if you try accessing the “wrong” methods on LocalDate you’ll effectively get a T00:00:00Z, but that’s not so bad), so I haven’t delved particularly deeply.

Sample Node transcript showing the problem and a hacky postprocessing fix for the prototype (though you’d need to do this recursively):

> toml = require("@ltd/j-toml");
…
> data = toml.parse("key = 'value'")
Object <[Object: null prototype] {}> { key: 'value' }
> Object.getPrototypeOf(data)
[Object: null prototype] {}
> Object.setPrototypeOf(data, null)
[Object: null prototype] { key: 'value' }
LongTengDao commented 2 years ago

Sorry for replying late, I have read you issue days before.

What's counter-intuitive, object without any proto is slower than object with a proto for tenfold. I can't give you ready-made https://jsperf.com link because the site is down... You can try below in https://jsbench.me/:

// setup
window.plain = {};
window.null = Object.create(null);
window.Null = Object.create(Object.create(null));

// test case 1: x10
'use strict';
const object = window.plain;
for ( let i = 1e6; --i; ) { object.key ??= null; }

// test case 2: x1
'use strict';
const object = window.null;
for ( let i = 1e6; --i; ) { object.key ??= null; }

// test case 3: x10
'use strict';
const object = window.Null;
for ( let i = 1e6; --i; ) { object.key ??= null; }

Object.create(null) will make more than tenfold slower, while Object.create(NULL) can regain to {} with proto properties safe.

But what you said is also important, which I didn't meet before. I want to know that, what bad thing will actually happen, when an object is not simple? (What do you mean of "uses the iteration prototcol"?)

I searched a popular lib merge-options and it's dep is-plain-obj, it seems thing should work right:

https://github.com/sindresorhus/is-plain-obj/blob/68e8cc77bb1bbd0bf7d629d3574b6ca70289b2cc/index.js#L7


How could you got a T00:00:00Z? I can't reproduce it or find any trace of it by the source code. Would you please give a use case to help to figure out that? If it exists, it's definitely not expected behaviour.

@chris-morgan