search

ajvincent / es-membrane

An ECMAScript implementation of a Membrane, allowing users to dynamically hide, override, or extend objects in JavaScript with controlled effects on the original objects.
ISC License
109 stars 13 forks source link

By default, es-membrane does a lot of whitelist-like lookups on the prototype chain (this may be undesirable) #169

Open ajvincent opened 6 years ago

ajvincent commented 6 years ago

Features such as filterOwnKeys, storeUnknownAsLocal, etc. exist for all es-membrane Proxy objects. This is going to hurt the performance of any non-distorted membrane proxies, probably measurably.

I should probably look into breaking up ObjectGraphHandler into two parts: the baseline ObjectGraphHandler, and a ChainHandler named WhitelistHandler.

ajvincent commented 6 years ago

In terms of creating object graphs initially, we have options now (literally). Membrane.prototype.getHandlerByName takes an options argument with one property, mustCreate. It would be easy to add a whitelist: true setting. But it would also be an across-the-board change for tests (which is okay) and for any current es-membrane users (which is probably okay, since I would send an advisory e-mail out, and since it's pre-1.0).

ajvincent commented 6 years ago

This should not be started until #172 is complete: we have ObjectGraphHandler and soon a PriorityQueueHandler. This will probably define source/ProxyHandlers/Whitelist.js as well.

tvcutsem commented 6 years ago

Since you asked for my opinion: yes, it makes sense to make whitelisting a pay-as-you-go feature that you don't pay for if you don't need it.

I also think making whitelist: false the default makes sense, even though I guess it would be backwards-incompatible.

That said, it's hard for me to gauge how much effort it requires to do this refactoring. Unless and until you have a heavy user of non-whitelisting membranes, it may not be worth the effort purely for performance sake (premature optimisation and all that...)