stampit-org / stampit

OOP is better with stamps: Composable object factories.
https://stampit.js.org
MIT License
3.02k stars 102 forks source link

Making instanceOf work for stamps #312

Closed far-blue closed 7 years ago

far-blue commented 7 years ago

ES6 has Symbol.hasInstance which allows you to control the function run to determine if an object is an instance of another object. Usually, as we know, it simply checks the prototype chain. If stampit kept a track of the objects / propertyDescriptors combined to create the objects returned from a stamp, those objects could have an overridden Symbol.hasInstance method that checks the list of combined objects / propertyDescriptors to determine instanceOf.

koresar commented 7 years ago

JFYI. You can write a stamp which would keep track of stamps any stamp is made of. Sounds insane. But easy doable. I'll do. 😊

On Tue, 23 May 2017, 00:37 Robert Goldsmith notifications@github.com wrote:

ES6 has Symbol.hasInstance which allows you to control the function run to determine if an object is an instance of another object. Usually, as we know, it simply checks the prototype chain. If stampit kept a track of the objects / propertyDescriptors combined to create the objects returned from a stamp, those objects could have an overridden Symbol.hasInstance method that checks the list of combined objects / propertyDescriptors to determine instanceOf.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/stampit-org/stampit/issues/312, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjCLxOs_2fM3bU2CkvFFmDVnvxfDVzgks5r8Z2mgaJpZM4Nicpe .

danielkcz commented 7 years ago

@far-blue https://github.com/stampit-org/stampit/issues/44#issuecomment-70051741

far-blue commented 7 years ago

My suggestion is to reuse a broken element of js to turn it into a helpful facility by replacing the original broken behaviour with well defined stamp-compatible behaviour without having to invent a new interface :) wouldn't rescuing instanceof and recycling it be a good thing?

danielkcz commented 7 years ago

@far-blue And did you ask yourself a question if it's actually needed? Stamps are so different concept than classes, that instanceOf is more of misleading than being helpful. If anything, it should be stampOf to denote the difference, but going with the same naming is just for confusing people.

danielkcz commented 7 years ago

Anyway, you are surely welcome to create a PR in our monorepo that would handle that. It's a kinda beauty of stamps that not everybody needs to use all crazy ideas someone has :)

far-blue commented 7 years ago

shrugs It doesn't impact me either way as it's easy enough for me to implement myself for my own use. I'm just suggesting ideas - such as extending what instanceOf actually does (rather than what people think it does) to cover multiple prototypal delegation using new capabilities in ES6 others may not be aware of.

But if you want to dismiss the idea and then categorise it as crazy I'll bow out and go back to my life :)

danielkcz commented 7 years ago

Well, so I don't understand why are you seeking this since it doesn't impact you :) Either it's something you want to use with stamps or you just don't want to bother to make a small contribution? Something like this doesn't need to be in a core of stampit nor as a part of the specification. It is a completely separated idea that is optional.

It's possible that what I find as a "crazy idea", it might actually make a transition to stamps easier for other people coming from Java-like languages that are used to this. Later they might realize it's not needed, but that's another story.

ericelliott commented 7 years ago

Since ES6 allows us to fix the broken instanceof behavior, I think it's not actually a terrible idea.

I'm open to discussing this as an addition to the specification. That said, the discussion should be in the specification repo, not here.

koresar commented 7 years ago

I'd argue this needs to be a specification change. It can also be a stamp!

Here is how how you can do it: scroll down to Collecting a stamp’s origins.

const InstanceOf = stampit()
  .composers(({stamp, composables}) => {
    // get or create the configuration metadata
    const conf = stamp.compose.configuration || {};
    stamp.compose.configuration = conf;
    // concatenate new composables with the previous list
    conf.wasComposedOf = composables.concat(conf.wasComposedOf);
    // de-duplicate the array
    conf.wasComposedOf = _.uniq(conf.wasComposedOf);
    const wasComposedOf = conf.wasComposedOf;

    // get or create the methods metadata
    const methods = stamp.compose.methods || {};
    stamp.compose.methods = methods;
    // add or overwrite the method which returns the composables
    methods.getOrigins = () => wasComposedOf;

    // get or create the statics metadata
    const statics = stamp.compose.statics || {};
    stamp.compose.statics = statics;
    // add or overwrite the `instanceof` method
    statics[Symbol.hasInstance] = (instance) => 
      _.union(instance.getOrigins(), wasComposedOf).length === instance.getOrigins().length)
  });

InstanceOf() instanceof InstanceOf; // true

const Derived = compose(Stamp1, InstanceOf, Stamp2);
Derived() instanceof InstanceOf; // true

Please ignore performance of the code. It can be improved any time. Spot the idea. Base on it.

danielkcz commented 7 years ago

@koresar It surely is possible to do it as stamp, but as @ericelliott said here it would be another implicit dependency you would need to add to every stamp to expect instanceOf working. Especially in this case if you don't compose with that stamp you would get the nasty surprise of instanceOf working differently. So yea, it should be probably part of spec.

On other hand it increases memory footprint for every stamp, so including it by default would influence everyone which doesn't seem that good.

koresar commented 7 years ago

it would be another implicit dependency you would need to add to every stamp to expect instanceOf working.

It's the only downside, which can be corrected on the module level (like "stampit").

But think about huge performance implications both CPU and Memory. Also think about majority of the users who don't need that feature. Why should they suffer? Because Eric Elliott changed his mind on the instanceof topic? :)

far-blue commented 7 years ago

Well, the details of which entities were used in the creation of a factory function would be stored in the factory function, not the objects it creates. Each factory would have a method each object it creates then references. The factory could hold (possibly weak) references to the factories or propertyDescriptors themselves and allow a recursive search or, alternatively, some form of uuid could be used and each factory could gather an array of all the uuid strings. The first likely uses less memory if you have thousands of stamps. The second is likely faster. A third option would be something like immutable.js's trie structure which could be shared and extended but that introduces a dependency.

koresar commented 7 years ago

@far-blue you have described exactly the code above (same code in this article).

The UUID solution is something very unique.


Few notes though.

far-blue commented 7 years ago

When all the additional memory use, little as it is, and cpu time is only consumed in the creation of the factories then the impact is going to be small.

Sorry, yes, you are right. It's a stamp descriptor object, not a propertyDescriptor. To be absolutely clear for everyone, I mean objects that look like this: https://github.com/stampit-org/stamp-specification#the-stamp-descriptor

koresar commented 7 years ago

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

ericelliott commented 7 years ago

Because Eric Elliott changed his mind on the instanceof topic? :)

Let's not get crazy. instanceof's default behavior is deeply problematic, and I have not changed my mind about that.

However, making instanceof work by looking up a string-based tag (NOT identity based) could work across execution contexts, could work even if a prototype changes (just declare that the new prototype satisfies some tag's contract), and basically has none of the problems with the original instanceof behavior.

When circumstances change, we're allowed to adapt our stance. In fact, if you don't adapt your stance, you could get knocked over. ;)

That said, if it disables hidden class optimizations, that sucks. Which reminds me, engineering tradeoffs and stupidity are often indistinguishable if you're unaware of the tradeoffs. Turning off optimizations globally due to custom instanceof behavior in local objects strikes me as stupidity. Clearly I'm unaware of some tradeoffs in V8. ;)

koresar commented 7 years ago

@ericelliott I'd test the new V8 v5.9. It has a bunch of optimizations. We might want to come back to this topic.

It's good you mention the ES6 Symbol for String Tags. I was just thinking about it yesterday.

Looks like String Tags would work in the current stamp spec:

$ node
> var o = Object.assign({}, {[Symbol.toStringTag]: 'MyFoo'})
o.toString()
'[object MyFoo]'

We could utilise that for the instanceof implementation.

    const stampStringTag = stamp.compose.properties[Symbol.toStringTag];
    statics[Symbol.hasInstance] = (instance) => 
      instance.toString() === `[object ${stampStringTag}]`

Or something like that.

ericelliott commented 7 years ago

A single instance could belong to any number of stamp type classes. Just space separate them in the string tag. Then you'll get a list of type classes satisfied by the instance. Cool?

ericelliott commented 7 years ago

In the short term, we could update a generic is util to see if an instance satisfies a stamp contract based on string tag.

koresar commented 7 years ago

A single instance could belong to any number of stamp type classes. Just space separate them in the string tag. Then you'll get a list of type classes satisfied by the instance. Cool?

Sorry, I didn't understand. When talking about code I prefer code. Can you show some code of how you gonna separate stamp descriptors (not stamps)?

ericelliott commented 7 years ago

An instance is not made of a single stamp. An instance is made of a whole bunch of stamps which may allow that instance to conform to a lot of different interfaces. When looking at instances, we typically want to know "does this thing support this interface I want to use to manipulate this thing?"

The code in question usually doesn't know about or care about what particular stamp the thing popped out of.

For example... do we really need to know if the thing we're looking at is an array, or can we deal with anything that has a working .map(), .concat(), and .reduce()?

Usually, it's the latter. Anything with a compliant version of those methods will satisfy the following interfaces:

Looking at the particular stamp that produced an object should generally be considered an antipattern.

Looking at the interfaces satisfied by the object is a much more reliable way to program. Remember the Gang of Four advice: "Program to an interface, not an implementation."

Looking for the name of a particular stamp is equivalent to looking for a particular implementation.

In other words, it's not very useful to have instanceof look for === equality for a particular stamp name.

Instead, we could have the string tag for the above say Functor Semigroup Foldable instead of MyArrayLikeStamp.

Then our instanceof code could do:

const hasInterface = interface => stringTag => stringTag.includes(interface);
koresar commented 7 years ago

"does this thing support this interface I want to use to manipulate this thing?" const hasInterface = interface => stringTag => stringTag.includes(interface);

That is similar to what my code above is doing. But it's using === reference equality instead of String.includes():

_.union(instance.getOrigins(), wasComposedOf) ...

Probably it's not the best solution. I'm more inclined toward the string tags as you suggest @ericelliott. It's more explicit, more developer and performance friendly.

Now, we need to solve last thing. What API will be giving a (interface) name to a stamp?

My initial proposal: The Named stamp. Tl;DR:

import Named from '@stamp/named'

const MyNamedStamp = MyRegularStamp.compose(Named).setName('MyNamedStamp')

But there could be better ES6 solutions.