stampit-org / stamp-specification

The Stamp Specification: Composables
434 stars 16 forks source link

Should static methods/props be assigned to a stamp's prototype? #104

Closed koresar closed 6 years ago

koresar commented 8 years ago

This is an open question. I have no strong opinion on that yet.

Sometime some tools check prototype of a class/function/component/stamp/etc. For example:

function isReactComponent(component) {
  return typeof component.prototype.render === 'function';
}

This would always return false for regular stamps. There is no standard way to attach anything to a stamp's prototype.

There are two solutions to this:

  1. Standardize that methods are also assigned as a stamp's .prototype. Or,
  2. Do not change anything and advise people to use infected compose for that need.

Currently, I'm inclined towards the second solution.

It would be great to hear React devs opinion.

danielkcz commented 8 years ago

I am not really sure that it should be statics that go to prototype. Why? Even mentioned render method is not static one, it's instance method.

koresar commented 8 years ago

@FredyC My late evening mind tricked me. Thank you for clarification. I've updated the issue. Now, it's says "methods" instead of "statics". Sorry about that.

JosephClay commented 8 years ago

Explicitly checking the prototype for something is bad practice IMO. Instead of seeing if the instance has the method, it's looking for the object to have a specific structure and the method.

Also, checking the instance for a top-level method will check the prototype chain.

Vote for 2

koresar commented 8 years ago

This is not about instance checking @JosephClay This is about constructor structure check.

> function F(){}
> F.prototype.render = function render(){};
> console.log(F.render);
undefined

You are probably confusing instance.__proto__ with a special ConstructorFunction.protoype object.

So, those tools I mentioned are checking Constructor.prototype but not instance.__proto__

unstoppablecarl commented 8 years ago

What specific tool is checking for that? I would think this is poor practice in general.

koresar commented 8 years ago

Yeah, it might not work with react HOC's for example. @FredyC can you remind which tool you stumbled upon?

danielkcz commented 8 years ago

It's react-redux-provide. However my first issue was about propTypes not being on a stamp (aka class), but that was only my mistake and those were there actually. That check for render doesn't seem to be actually that much relevant.

General question here is if stamps should be compatible with classes in how they look like to outside world. I assume it will be some time before stamps can actually replace classes.

unstoppablecarl commented 8 years ago

I don't think it will be possible to make them 'look' like classes. They are not classes. I think a user would need to make some sort of adapter implementation for those cases. Trying to get close to looking like and behaving like a class based on our assumptions of usage is probably not going to be helpful for those cases anyway.

I think this type of thing will add to the complexity and confusion of learning and understanding stamps in general.

danielkcz commented 8 years ago

Yea, it's not really a pressing matter right now, this was only single case I've run into and so far doesn't seem relevant. Let's keep it open here in case we find some other reason for this.

ericelliott commented 8 years ago

If you assigned methods to stamp.prototype by identity, it would enable instanceof checks, which IMO, is a bad thing.

danielkcz commented 8 years ago

I don't think that works like that simply because stamp doesn't have a constructor property which is used for instanceOf check.

ericelliott commented 8 years ago

The constructor property is not involved at all in the instanceof check. See my answer to Confused about javascript prototypal inheritance.

Verify for yourself:

function foo() {}
var bar = { a: 'a'};
foo.prototype = bar; // Object {a: 'a'}
baz = Object.create(bar);
baz instanceof foo // true

Note: No constructor property anywhere...

unstoppablecarl commented 8 years ago

The constructor property is a developer convention added for convenience.

danielkcz commented 8 years ago

Ok I did not know about that, thanks.

Either way, let's keep it open here for now. Since this came from a react-stamp, it's more likely end up there and perhaps with just making copy of a render method on a prototype to fulfill basic needs to identify components.

koresar commented 6 years ago

Bad idea. :) No outcome. Closing the issue.