Using a dictionary instead of an object?

[WebReflection]

The following easily fails:

const tlru = require('tiny-lru');
const cache = tlru();
cache.get('toString');

and this is a common issue when objects, instead of dictionaries, are used in conjunction of the in operator.

In NodeJS, they've benchmarked that the best way to go is:

function Dict() {}
Dict.prototype = Object.create(null);

Once you have that, instead of this.items = {}; you'd go this.items = new Dict; and you grant no name clashing through the prototype could possibly ever happen.

Thoughts?

[avoidwork]

Does it change the deopt? The (single) issue with the current design is that cache items become megamorphic.

[WebReflection]

Not sure I'm following, but the proposed changes fixes a bug and for the rest would work the same.
This issue is well known and documented, in NodeJS query string parser the __proto__ attack was causing disasters and here you'd have the same.

It's really not an option to use literals like dictionaries.

I'd use either a Map instead, or go real dictionary

[avoidwork]

So this is not based on performance, but instead the prototype chain? I'm not interested in any such change; Maps are too slow for a cache.

[WebReflection]

you have at least 12 breaking keywords for this cache

"toSource"
​"toString"
​"toLocaleString"
​"valueOf"
​"hasOwnProperty"
​"isPrototypeOf"
​"propertyIsEnumerable"
​"__defineGetter__"
​"__defineSetter__"
​"__lookupGetter__"
​"__lookupSetter__"
​"__proto__"
​"constructor"

plus others potentially introduced by libraries extending, for whatever reason, the Object prototype.

There is a bug, and it looks like you are not willing to fix it.

I guess a fork is the next step.

Regards

[avoidwork]

Guess how many issues have been filed on those words. The scenario you're considering doesn't block you from someone already overriding Object.create(); this is a straw man.

[WebReflection]

The scenario you're considering doesn't block you from someone already overriding Object.create();

if executed once and the module is early required it's safe.

I don't understand why anyone would like a bug super easy to solve in their code.

NodeJS core team fixed their objects used as cache like you are doing here, because there is a precedent for the described issue.

[avoidwork]

Zero issues filed; if you don't sanitize keys in your code you're doing it wrong.

[avoidwork]

This is also core design of the language, while Object.create() was created for a single selfish reason; if you don't trust the prototype chain don't run javascript.

[WebReflection]

if you don't sanitize keys in your code you're doing it wrong.

I am using a cache, which should act like one. There is a bug in your code, feel free to ignore it.

Maps are too slow for a cache

This is also quite a bold statement, as Maps these days can be even faster.

while Object.create() was created for a single selfish reason; if you don't trust the prototype chain don't run javascript.

Nobody said you don't trust the Object prototype, the bug is about the existing prototype already.

Also, the moment __proto__ passes through, goodbye your cache.

Once again, it's your project, I'll just try to be sure I won't include it anywhere, since this conversation makes no sense for such popular module.

Best Regards

[avoidwork]

The "bug" is the language. By using those strings as keys you'd block access to the prototype methods of Object; no class methods utilize those keys, so the scenario you're describing is outside of the bounds of what this library is concerned with.

If the dictionary mode solves the deopt, I'm super interested in it. If you don't know what this is, you're not really looking at how it works.

[WebReflection]

By using those strings as keys you'd block access to the prototype methods of Object

No, the module fails, as already described in the first post.

const tlru = require('tiny-lru');
const cache = tlru();
cache.get('toString');

do you understand there is an error there that shouldn't, since any other key wouldn't fail?

[avoidwork]

If that's a real world problem, open a PR. Its impossible in my work so I don't care to do it. I'm not going to block you from landing it.

[WebReflection]

I would've landed a PR after a normal conversation, as I've already told you what I would've done differently, and asked your opinion about it.

But after all this, I'm now convinced I shouldn't care about this module and actually stay away from it.

What a sad interaction.

Best Regards.

[avoidwork]

This is a normal conversation, from my perspective. You asked a question, and I answered truthfully, and provided my reason.

The entire outcome is by your doing; other people simply open PRs and describe why it's useful, not present a language design as a bug.

[avoidwork]

For example, avoidwork/filesize.js#102 ... that person didn't ask a question, just opened a PR ... totally frictionless. blaming me for not liking hearing 'no' is a failing on your part, and also not understanding that i can say no and not be a dick.

[WebReflection]

Just to be clear:

• using an object literal as trash bin has never been a good idea in general, that is why Map was introduced in the language
• assuming you can be careless and use the object literal, you should not use the in operator, because it reveals prototype chain details
• even using items.hasOwnProperty would be a bad choice, 'cause hasOwnProperty could be used as cache, so that having hasOwnProperty.call(items, key) would be at least safer
• because of all these gotchas described in previous points, they introduced Object.create(null) to exactly address all these common hidden caveats with objects literals (and thanks gosh it works on prototype too)
• as result, you are using the wrong primitive to obtain unsafe, easy to break, results

I've been working and contributing in Open Source for 20 years, and just landing PRs is not usually welcome, because devs might have reasons to pick one approach instead of another.

And yet, you are blaming me for opening a bug and discussing a possible solution before opening a PR ... so yes, you've been a dick (since you mentioned that part).

Best Regards.

[avoidwork]

^ could've opened the PR in that time frame. You're hung up on me not caring as much about a potential issue as you do. Your bias is only your's.

[avoidwork]

The truly telling thing for me is you'd rather argue & fork, then make the 1 liner.

[StarpTech]

@avoidwork I read the conversation and I'm a bit shocked. @WebReflection found a neat bug and even provide a solution. What's missing? The bug is obviously a name clashing scenario because of the "in" operator.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/in#Inherited_properties

[avoidwork]

Shocked? I was asked for an opinion, and I gave one. Why does this problem even exist in your code? What's the scenario that this actually manifests? I can randomly delete attributes or methods and break the cache the same.

[StarpTech]

Shocked? I was asked for an opinion, and I gave one. Why does this problem even exist in your code?

It doesn't exist but that's not the point. This issue was opened to prevent it.

What's the scenario that this actually manifests? I can randomly delete attributes or methods and break the cache the same.

@avoidwork don't start hypothetical scenarios when this issue already happened in our community. We are talking about some lines which will improve the overall reliability of this module.

[avoidwork]

You're free to make the change. You guys are bringing a hypothetical situation to me. I purposefully chose the operator knowing the issue that it could expose. I don't make code* that protect devs from themselves, but you're able to make those changes if you want

[avoidwork]

I'm also gonna lock this; anyone can open a pr if they want to land a change.