Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Static collisions & modularization #149

Closed
ericelliott opened this issue Jul 8, 2015 · 59 comments
Closed

Static collisions & modularization #149

ericelliott opened this issue Jul 8, 2015 · 59 comments

Comments

@ericelliott
Copy link
Contributor

New Hotness by @koresar

import stampit from 'stampit'; // <- stampit v2 compatible
import {compose, init, props, methods, stamp } from 'stampit'; // <- the new functional way, v2 incompatible

Pros:

  • No static collision danger to worry about
  • Is functional
  • Probably faster
  • Not a breaking change
  • User has choice over import statement trade-off

Cons:

  • this space intentionally blank

👍 👏 😎 🎸 🔥 🚒 🍰

Original discussion:

See #148

Method .init returns nothing after .compose & .convertConstructor #148

Our chaining convenience methods are causing real problems for real users trying to build things with stampit. I proposed the solution of modularizing our tools out of the stamp statics:

import {
  compose, props, methods, init
} from 'stampit/tools';

compose(
  props({body: {...}),
  methods({...}),
  init(() => {...}),
  Prevalidable
}).prevalidate({put: {body: joi.required()}});

Pros:

  • No static collision danger to worry about
  • Is functional
  • Probably faster

Cons:

  • larger import statement

@koresar suggests guarding statics by composing another stamp:

export default GuardStatics = stampit().static({ static: function(staticProps) {
  return this.static(_.omit(staticProps, ['init', 'props', 'refs', 'methods', 'compose', 'create']));
});
RestClientFactory
  .compose(GuardStatics, Prevaildable)
  .props({body: {...}})
  .methods({...})
  .init(() => {...})
  .prevalidate({put: {body: joi.required()}});

Pros:

  • Can hack around the collision problem
  • Smaller import statement, and no import is needed when composing stamp objects
  • No breaking change needed

Cons:

  • Doesn't eliminate collision danger
  • Work-around is much less intuitive (it's a work-around, not a solution)
  • Now we have a user education problem -- a gotcha users must avoid that requires somewhat advanced knowledge of stampit
  • Less modular approach
  • Doesn't mix invisibly with functional programming
  • Probably slower
@koresar
Copy link
Member

koresar commented Jul 8, 2015

The possible GuardStatics implementation (warning, it's not complete!):

var GuardStatics = stampit().static({ static: function(staticProps) {
  return this.static(_.omit(staticProps, ['init', 'props', 'refs', 'methods', 'compose', 'create']));
});

@ericelliott
Copy link
Contributor Author

👍 Added to description. =)

@ericelliott
Copy link
Contributor Author

@troutowicz @unstoppablecarl @JosephClay Pretty big breaking change API discussion here.

Definitely want more eyes on this.

@mdhooge
Copy link

mdhooge commented Jul 8, 2015

Still thinking about it…

Eric's solution sounds really better to me because it puts all stampit methods into a separate namespace. So there is no risks, if a stamp is handed to someone else, that he will gets confused. Because, as I see it, we can't expect one .init() method to be "more important" than the other (to re-use #148 issue).

@ericelliott
Copy link
Contributor Author

I'm seeing a pretty large growing list of cons for the workaround...

Am I leaving anything out of either pros/cons argument?

@koresar
Copy link
Member

koresar commented Jul 8, 2015

Introduces _.omit dependency

Remove this line please. :) This was a pseudo code. Not a real code. The real code will have no dependencies.

@koresar
Copy link
Member

koresar commented Jul 8, 2015

Doesn't mix invisibly with functional programming

The entire stampit philosophy is not about functional programming. It's about objects which have both state and methods.

Please, remove this statement as irrelevant.

@koresar
Copy link
Member

koresar commented Jul 8, 2015

Now we have a user education problem -- a gotcha users must avoid that requires somewhat advanced knowledge of stampit

I was thinking to guard stamp's methods inside the stampit itself, so this problem would never arise. However, that's the way @troutowicz implemented statics.

@koresar
Copy link
Member

koresar commented Jul 8, 2015

Work-around is much less intuitive (it's a work-around, not a solution)

That's not a workaround which @mdhooge was talking about. He's using .enclose() alias. Please rephrase this statement.

@ericelliott
Copy link
Contributor Author

I was thinking to guard stamp's methods inside the stampit itself

So end-users don't have to know about GuardStatics? How will that work, exactly?

@ericelliott
Copy link
Contributor Author

That's not a workaround which @mdhooge was talking about. He's using .enclose() alias. Please rephrase this statement.

That statement is not related to the .enclose() workaround. I'm talking about GuardStatics - which is also a workaround.

@ericelliott
Copy link
Contributor Author

Workaround = work you have to do to route around a problem.

vs

modularization: problem disappears completely, so you don't have to write any special code to handle it.

Noteworthy, the latter also has perf benefits, because the extra GuardStatics code does not have to be interpreted or invoked, ever.

@mdhooge
Copy link

mdhooge commented Jul 8, 2015

Maybe it's already time to introduce stampit v3 ;-)

As I see it, it is still possible to inject the extracted methods into a stamp. So the existing code base is still valid.

@koresar
Copy link
Member

koresar commented Jul 8, 2015

So end-users don't have to know about GuardStatics? How will that work, exactly?

:)))
I'll flip 2-nd and 3-rd argument in this mixin function call. And done! :)
https://github.com/stampit-org/stampit/blob/master/src/stampit.js#L189

@mdhooge
Copy link

mdhooge commented Jul 8, 2015

I don't know the purpose of EventEmitter.init() method. But how can we be sure that it will still operate properly if we put stampit method instead?

Maybe the only problem is convertConstructor

@ericelliott
Copy link
Contributor Author

Well, yes, I agree you should flip the 2nd and 3rd argument, but then you've just shifted the problem from one side of the equation to the other. It's still a problem. Statics still collide -- that just changes what happens when they do...

@ericelliott
Copy link
Contributor Author

Maybe the only problem is convertConstructor…

A necessary evil. Constructors suck, but there are lots of constructors because there are lots of people who don't know that constructors suck. ;)

@koresar
Copy link
Member

koresar commented Jul 8, 2015

compose(
  props({body: {...}),
  methods({...}),
  init(() => {...}),
}).prevalidate({put: {body: joi.required()}});

On top you are missing 'Prevalidable` stamp to be composed with.

@koresar
Copy link
Member

koresar commented Jul 8, 2015

This all comes to #80.

We can have refs, props, compose, init, methods as keywords, as a language syntax.

@ericelliott
Copy link
Contributor Author

We can have refs, props, compose, init, methods as keywords, as a language syntax.

It would probably be Object.* and the names would almost certainly be changed due to design-by-committee.

Actually, maybe even Object.stamp.*.

@koresar
Copy link
Member

koresar commented Jul 8, 2015

Probably faster

No, it is not.

I know stampit internals. :) It'd be same. The main performance bottlenecks are various mixin and forEach functions.

Probably slower

No it is not by the same reason.

Please, remove both. Thanks.

@ericelliott
Copy link
Contributor Author

No, it is not.

It is if the user has to add code to work around static collision. We either override one or the other, and to avoid that override, the user will need to make a reassignment, and that's at minimum an additional line of code, and at worst a mapping reassign over several props.

And that happens every time the stamp in question is composed with other stamps.

Worst case scenario, 6 reassignments * n compositions.

@koresar
Copy link
Member

koresar commented Jul 8, 2015

I'd rather be lodash like.

import _ from 'lodash'; // <- imports entire module
import {map, forEach, pick, filter} from 'lodash'; // <- imports few methods only

stampit:

import stampit from 'stampit'; // <- stampit v2 compatible
import {compose, init, props, methods} from 'stampit'; // <- the new functional way, v2 incompatible

How about that?

@koresar
Copy link
Member

koresar commented Jul 8, 2015

Maybe even have something like "babel/register" but:

require('stampit/register');
Object.compose;
Object.init;
Object.props;
Object.methods;

// or maybe even as global variables :)))
compose;
init;
props;
methods;

@koresar
Copy link
Member

koresar commented Jul 8, 2015

This talk is coming to class-like syntax support. (Eric, this is a pseudo code, not something well-though and tested.)

stamp StampName {
  init() {} // .init
  method1() {} // .methods
  static method2() {} // .static
  get reference1() {} // .refs
  set reference1() {} // .refs
  property1: { deep: 'value' } // .props
}

stamp ComposedStamp composes StampName, SomeOtherStamp; // like "extends" in classes

@koresar
Copy link
Member

koresar commented Jul 8, 2015

Less modular approach

@ericelliott would you elaborate on that? How more modular stampit can be? (I'm really curious.)

@ericelliott
Copy link
Contributor Author

import stampit from 'stampit'; // <- stampit v2 compatible
import {compose, init, props, methods} from 'stampit'; // <- the new functional way, v2 incompatible

I'd add stamp which is stampit as a pure function without the statics:

import { compose, init, props, methods, stamp } from 'stampit';
stamp composedStamp from StampName, SomeOtherStamp; // `from` instead of `composes`?

@ericelliott
Copy link
Contributor Author

import stampit from 'stampit'; // <- stampit v2 compatible

👍 😎

@ericelliott
Copy link
Contributor Author

  • More chances to collide with users' variable names compose, props, methods, init, refs.

No, because the user decides the names. ;)

@ericelliott
Copy link
Contributor Author

One minor nitpick: Any stamp that uses the v2 compatible will infect any composed stamps with collision danger. We should mention in the doc that users should prefer the modular version if they're making a stamp that will likely be used as a general-purpose mixin.

@ericelliott
Copy link
Contributor Author

Action Items

  • New Hotness tests
  • Implement new hotness
  • Document mixin collision warning for v2 compat API

@koresar Do you want to implement?

@troutowicz
Copy link
Member

I like the new hotness. 👍 Better to fix the problem (static collision) then to implement workarounds.

I don't really consider the import statement trade-off a big deal. If the developer wants to type less:

import * as stampit from 'stampit';

const validatedStamp = stampit.compose(
  stampit.props({body: { ... }),
  stampit.methods({ ... }),
  stampit.init(() => { ... }),
  Validator
}).prevalidate({ put: { body: joi.required() } });

That does not look pretty when using lots of shortcut methods but may be fine in situations where only one or two shortcut methods are used.

To be honest, with this new API, @koresar's production example is better off being rewritten:

import * as stampit from 'stampit';

const validatedStamp = stampit.compose(
  stampit.stamp({
    init() { ... },
    props: { body: { ... } },
    methods: { ... }
  }),
  Validator
).prevalidate({ put: { body: joi.required() } });

This would perform faster and is cleaner.

@JosephClay
Copy link
Contributor

Sorry guys, just getting to this now.

IRL, I use stampit much like @koresar does. The immutable chaining makes composition clearer and (most importantly) easy to learn.

Any terms of what is "faster" should be ignored as a pro. Object construction isn't/shouldn't be a pain point in an application and the current version performs well.

I'm fine with the workaround as long as there's still the option to use v2 as is. Modularization to the point of importing 5+ functions works, but puts the burden of setting up the libs' surface area on the user every time. To me, it feels like an extreme and so is still a workaround.

Also, stampit is offered over a CDN, so the ES5 equivalent code to the above functional import version would be:

var stampit = window.stampit; // do not use!
var compose = stampit.compose;
var init = stampit.init;
var props = stampit.props;
var methods = stampit.methods;
var stamp = stampit.stamp;

@JosephClay
Copy link
Contributor

@troutowicz I'd argue that the example is not cleaner. I would rather import a validator stamp and, if I'm not composing multiple stamps, use stampit's immutability to create a new stamp.

import validator from './validator';

const validatedStamp = validator
    .init(function() { ... })
    .props({ body: { ... } })
    .methods({ ... })
    .prevalidate({ put: { body: joi.required() } });

@troutowicz
Copy link
Member

That's fine, I'm not arguing against the usefulness of stampit's current immutable shortcut methods. This issue is about static collisions, your example does the exact same thing as @koresar's and does nothing to prevent static collisions.

Issue #148 is why this issue was created. It shows, despite how useful they are, why stampit's statics can cause unexpected problems.

@JosephClay
Copy link
Contributor

I'm not arguing that the fix won't work. It just feels like a hefty solution to a problem I don't see occurring often. stampit doesn't have a ton to protect (7 properties?).

If we implement a warning when passing a colliding static and not overwrite stampit's static, then the user can work around the issue.

// pseudo code
import stampit from 'stampit';
import { EventEmitter } from 'EventEmitter';

const EventEmittable = stampit.convertConstructor(EventEmitter)
    .init(() => {
        EventEmitter.init.call(this);
    });

Pros:

  • Easy to catch collisions
  • Puts the power in the user's hands
  • No change to API

Cons:

  • It's hacky, but converting a constructor is already hacky.

@JosephClay
Copy link
Contributor

Sample warning logic (not tested)

@ericelliott
Copy link
Contributor Author

@JosephClay We've already been over this, and the solution is the best of both worlds. Updated issue description with "New Hotness" details.

@koresar
Copy link
Member

koresar commented Jul 10, 2015

The discussed functional approach does not solve the static collision of the .fixed property.

@ericelliott
Copy link
Contributor Author

We could rename it to stamp._stamp_.fixed, which is unlikely to collide with anything.

Of course, we'll need to deprecate stamp.fixed. =)

@koresar
Copy link
Member

koresar commented Jul 10, 2015

The proposed changes are too radical. Every bit of the new module will be incompatible with both v1 and v2. We should consider a separate package for that. "stampit-functional" or alike.

@ericelliott
Copy link
Contributor Author

The proposed changes are too radical

That's why we'll use a deprecation process. Look at it as a long-term journey. We can deprecate now, offer old-stamp deprecation console warnings for a full year (maybe two), and then release a Breaking change to remove support for stamp.stamp.

AFAIK, changing the name of stamp.fixed is the only radical breaking change being proposed. Am I missing something?

Toward an open-standard for Stamps

Eventually, I would like stamps to be an open standard implemented by potentially many libraries, not just stampit. Adoption will suffer if users can't use the properties they want to use.

@ericelliott
Copy link
Contributor Author

We should consider a separate package for that.

We can't have an open standard that not even we respect. I'm OK with keeping a chainable version going for people who prefer the legacy API because that is opt-in, but it should be possible to produce stamps that should never collide with userland libraries -- without forcing users to implement a hacky work-around.

In other words, both API's should use stamp._stamp_.fixed, or some alternative that is equally unlikely to collide.

@troutowicz
Copy link
Member

it should be possible to produce stamps that should never collide with userland libraries

👍

@koresar
Copy link
Member

koresar commented Jul 10, 2015

I like the idea of Open Stamps Standard.

Take a look at promises: https://promisesaplus.com/
It operates the so-called "then-able" objects. Should we come up with "create-able"?

_.isFunction(someObject.create); // true
_.isArray(someObject.create.inits); //true (we should rename the "init()" somehow.
_.isObject(someObject.create.refs); //true
_.isObject(someObject.create.props); //true
_.isObject(someObject.create.methods); //true
_.isObject(someObject.create.statics); //true

@ericelliott
Copy link
Contributor Author

Whoah. Mind. Blown. .create.* is a great solution to this problem.

@ericelliott
Copy link
Contributor Author

@troutowicz @unstoppablecarl @JosephClay Thoughts?

@koresar
Copy link
Member

koresar commented Jul 10, 2015

In the Open Standard the word "stamp" is not going to be understood straight enough. Take promises/futures for example. They are widely known concepts.

In the Open Standard we should call our stamps "functional mixins". It will be easier to understand, and also there is a widely known concept.

@troutowicz
Copy link
Member

I like .create.*. It would fix static collisions while, I think, making everyone happy.

functional mixins

What about "composable factories"?

@koresar
Copy link
Member

koresar commented Jul 10, 2015

What about "composable factories"?

Also good. Even better I think. :)

@koresar
Copy link
Member

koresar commented Jul 10, 2015

Thinking of Open Standard.

Functions are the first class citizens in JavaScript. I was worried all the time that stamps cannot produce functions as object instances.

There is always a workaround via .init(() => { return function () {}; })
But as Eric says

it's a work-around, not a solution. Now we have a user education problem


I believe that the .create() function should accept not refs object, but the object itself.

MyStamp(); // creates new object

const plainObject = { key: value };
MyStamp(plainObject); // returns the plainObject object

const myFunc = function whatver() {};
MyStamp(myFunc); // returns the myFunc object

Thoughts?

@koresar
Copy link
Member

koresar commented Jul 10, 2015

More thinking about Open Standard.

  • If stamp.create() argument is not an object (lodash check should be used _.isObject) then no refs or props should be added obviously. But all the init() functions should be called regardless.

In other words, stamps should flawlessly process any object. No exception, no warning, nothing. The type of the processed object should be up to the user. (Just like promises do.)

In the following example I'm using sound processing terminology:

const Cutoff = compose(
  refs({ min: 5, max: 250 }), 
  init(function({ instance }) { 
    return instance < this.min ? this.min : 
      intance > this.max ? this.max :
      instance; })
);
const Aplify = compose(
  refs({ factor: 2.5 }), 
  init(function({ instance }) { return this.factor * instance; })
);

const Amplificator = compose(Aplify, Cutoff);
Amplificator(200); // returns 250 because 200*2.5 is more than 250 cutoff upper limit
Amplificator(20); // returns 50 beacuse 20*2.5 is within the cutoff range 5-250
Amplificator(0); // returns 5 because 0 is less than 0 cutoff lower limit

@ericelliott
Copy link
Contributor Author

We are not changing the name, "Stamp." So it is written. So it shall be.

functional mixins
What about "composable factories"?

NO

If you think we have it tough explaining problem now, imagine a world where the name isn't:

  • Written about in a published O'Reilly Book
  • Installed and used in tens of thousands of apps
  • Evangelized for a couple solid years
  • Talked about in an influential fluent talk with 20k+ views (more than Brendan Eich's keynote from the same year)
  • Talked about in the seminal "Two Pillars of JavaScript" blog post that has been read over 100,000 times (that's more than most JavaScript books sell copies in their entire lifetime)

If you think it would be easy to reproduce the branding work that has already been done for stamps, you're sorely mistaken.

But they're so sparkly and new! ✨ 💖

functional mixins
What about "composable factories"?

These aren't names. They're descriptions. I'm happy to discuss alternative descriptions, but we're not changing the name, stamp. Of course people are asking "WTF is a stamp?" but we can't co-opt the already existing term "functional mixins" and try to make it mean something different, and "composable factories" is what stamps are, not what they're called. Nobody knew what a functor was before it was invented. Nobody knew what "JavaScript" was before 2006. ;)

Branding starts with a brandable name, and "stamp" is a brandable name. Want people to stop asking what it is? Then we need to make stamps ubiquitous, and we need to make sure the community has a lot of great learning resources.

Anybody know somebody who makes amazing explainer videos? ;)

@troutowicz
Copy link
Member

These aren't names. They're descriptions.

I thought @koresar WAS talking about an easy to understand description for the specification. Oops.

@koresar
Copy link
Member

koresar commented Jul 11, 2015

Yep. In the gitchat we all agreed that there's no need to change the name.

@koresar
Copy link
Member

koresar commented Jul 25, 2015

This issue was overtaken by the repo. I'm not seeing the original idea implemented.

I propose to close this issue. Any objections?

@ericelliott
Copy link
Contributor Author

Close in favor of implementing the spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants