stampit-org / stamp-specification

The Stamp Specification: Composables
434 stars 16 forks source link

Use `Symbol.hasInstance` to fix `instanceOf` operator #114

Closed danielkcz closed 7 years ago

danielkcz commented 7 years ago

Based on discussion in https://github.com/stampit-org/stampit/issues/312.

Specification MDN Dr. Axel Rauschmayer

Apparently, the Symbol.hasInstance can be used for a function too. That could be useful to actually check if stamp is composed of some other stamp by using instanceOf

Support is not that terrible. A question is what happens if used in an environment without support. It would break the code and it would be really hard to track such issues.

koresar commented 7 years ago

That's great proposal!

It would help some Java-minded people at least.

Although, it might benefit only minority of stamp users. Like, 1%. :) Also, it deoptimizes V8: "Install Symbol.hasInstance property anywhere disables fast-path globally for the VM". And there can be other performance implications, like memory. So, we should make this feature optional.

We should accept the "composers proposal" - #109. It's a powerful universal solution which would give multiple abilities. To name a few:

This means that if someone want a feature like instanceof - he can simply compose it into the base stampit. Untested code of ./stampit.js or something:

import _stampit from 'stampit';
import InstanceOfStamp from `@stamp/instanceof`;
export default _stampit.bind(undefined, InstanceOfStamp);

We can even add a simple API to stampit. Something like: stampit.extendBaseStamp(...composables). So that if anyone would need instanceof in the entire application then just stampit.extendBaseStamp(require('@stamp/instanceof')) on the app startup. And done!

=====

Wrap up. Proposed change is a performance killer. Let's make it optional using the composers #109 feature.

danielkcz commented 7 years ago

We can even add a simple API to stampit. Something like: stampit.extendBaseStamp(...composables). So that if anyone would need instanceof in the entire application then just stampit.extendBaseStamp(require('@stamp/instanceof')) on the app startup. And done!

I certainly like this idea, it might actually work like babel or eslint presets and just go like stampit.inject('instanceof') which would do require in behind.

Anyway you are right that adding this to spec for everyone is probably not a good idea.

koresar commented 7 years ago

I'd like to remind that instanceof would rely on the composers feature. Which is experimentally implemented atm in the "stampit" module.

danielkcz commented 7 years ago

@koresar What about reminding that in a relevant issue? :) Maybe even create PR that would update the spec. I still haven't used composers for anything, so it's rather alien to me.

koresar commented 7 years ago

Thanks for the reminder.

If you used the Privatize then you used composers. :)