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

NULL prototype thing causes trouble #24

Open
chris-morgan opened this issue Nov 10, 2022 · 1 comment
Open

NULL prototype thing causes trouble #24

chris-morgan opened this issue Nov 10, 2022 · 1 comment
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@chris-morgan
Copy link

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 LongTengDao added the enhancement New feature or request label Nov 27, 2022
@LongTengDao
Copy link
Owner

LongTengDao commented Nov 27, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants