stampit-org / stamp-specification

The Stamp Specification: Composables
434 stars 16 forks source link

Avoiding collisions of stamp's options argument #26

Closed koresar closed 8 years ago

koresar commented 8 years ago

So, now we have standardised instance and stamp properties of the stamp's "options" argument.

let myObject = myStamp(options);

I afraid we can get property name collisions (again) in the future. The following code is a WTF code:

let myStamp = init(({ instance, stamp }) => {
  console.log(instance); // will not print "I am instance" !
  console.log(stamp); // will print "I broke your code" !
});
let myObject = myStamp({ instance: "I am instance", stamp: "I broke your code" });

As long as we ditched "args" array in favour of "options" object (so that "convertConstructor" became just one more behaviour - Yay! this is so cool!) I believe we can and should afford two arguments to stamps and three to init functions:

let myStamp = init(({ opt1, op2 }, instance, stamp) => {
  console.log(instance); // will print "I am instance" as expected
  console.log(stamp); // will print the stamp object as expcted
});
let myObject = myStamp({ opt1: 1, op2: 2 }, "I am instance");

Justification:

let myStamp = init(({ opt1, op2 }) => {
  // ...
});
let myObject = myStamp({ opt1: 1, op2: 2 });

Do you like it as much as I do?

ericelliott commented 8 years ago

I'm not sure this is a good idea. One of the best features of the options object is that it gives the stamp creator more control over the function signature. I think it's OK to have a few reserved properties on the options object. I'm not as sure it's OK to have a few reserved arguments in the stamp function signature.

koresar commented 8 years ago

If you were a TC39 memeber what would you standardize?

ericelliott commented 8 years ago

I'd stick to the options-object only approach. Additional options are superfluous and there isn't enough motivation to hamper the stamp function signature. There's nothing wrong with a handful of reserved properties, any more than there's anything wrong with a handful of reserved keywords in a programming language.

ericelliott commented 8 years ago

Are you satisfied enough with my feedback to close this issue? My major concern is that users of the spec are not building stampit. They're building networking libraries, or streaming libraries, or db connection libraries, etc... and they may have need for control over the arguments.

An options object is almost universally useful. However, an instance argument would not be broadly useful across many problem domains. In fact, I've only very rarely wanted the option in Stampit.

stamp is even less universally useful than instance. Why would somebody instantiating a db connection want to pass instance or stamp arguments?

The answer is simple: They wouldn't. However, they may want to use arguments for other things, such as a queue of signals to send across a network connection once the connection has been established...

But they can't do this:

const db = dbStamp({ url, credentials }, ...messageQueue);

Because the stamp is expecting an instance to be the second parameter... so we force them into awkwardness like this?

const db = dbStamp({ url, credentials }, null, null, ...messageQueue);

And in the dbStamp docs the author has to write: "the second and third parameter are not used, so pass nulls instead".

Of course, they could just use the options object for everything, but if certain patterns are common enough, maybe it makes sense to save people some typing by offering a second or third optional parameter. My point is, I don't believe it's our place to dictate function signatures for stamp creators.

koresar commented 8 years ago

Wait a second! Since when we allow any number of parameters to be passed to the factories/stamps? This is stamp-specification repository, not stampit. For now we allow only one parameter.

If you are talking about oldschool constructors then I should remind you the idea you expressed earlier and we both liked it. - The convertConstructor will be a utility function which implements the specific behavior.

import {convertConstructor} from 'stamp-utilities-module-of-some-kind';
const mqStamp = convertConstructor(MessageQueue); // old school ctor
const queue = mqStamp({args: {MessageQueue: messageQueue}}); // <- the idea is by Eric Elliott

This allows us to compose multiple oldschool constructors. Cool!


You have completely misunderstood my idea, Eric. :)

Please, take a closer look.

1) I did not propose to pass THREE parameters to stamps. But TWO at max. Quoting you:

an instance argument would not be broadly useful across many problem domains.

Yes! Yes! Yes! That's why I proposed it to be the second optional parameter to stamps/factories. composable([options], [instance]) - third and more arguments will be ignored.

2) <- will type in few hours. I'm busy atm.

koresar commented 8 years ago

Sometimes I think there are two Erics. :)

ericelliott commented 8 years ago

I think the point I'm trying to make is that currently, the spec is open for extension that would allow additional arguments.

My biggest regret about the Stampit API design was arbitrarily deciding that the first stamp argument would be used to initialize properties. That's usually what I wanted, true, but it wasn't always what I wanted.

Every decision we make about the stamp function signature is a possibility we take away from the stamp author. I think some well-communicated reserved properties of an options-object is an OK compromise.

koresar commented 8 years ago

The stamp's signature is open for extension via the first argument - options.

Take a look at Promises A+.

I mean that I'm strongly against "extending" the composables standard.

The standard must be minimalistic enough. The tools around it must be as comprehensive as possible. If a tool would like to prohibit instance, stamp, args properties in the options object - then let it be.

But the standard must not have any compromises.

Do you rememebr collitions of the stampit v2 static functions? We are going to hit collisions again if we forbid at least tiny portion of options object freedom.

I propose: 1) Standardize two parameters to the stamps. Both optional. Never extend it with the third parameter. 2) If someone would ever need some additional features from the composables - they'd need to implement it as a composable behavior. Basta.

koresar commented 8 years ago

I'd like you to answer the few following questions:

1)

let stamp1 = compose({ init({ instance, stamp }) => {
  console.log(instance);
  console.log(stamp);
}});

What will be printend if in the following case?

stamp1({ instance: { a: 1 }, stamp: { b: 2 } });

2) And please, take your time to answer this question thoroughly:

Why?

Why the two console.log print such output, but not else?

Thank you.

JosephClay commented 8 years ago

I can see where @ericelliott is coming from.

@koresar Taking a look at Promises A+ .catch() isn't part of the spec. It exists because using the error param in the .then() is an anti pattern. What kind of hideous API would we have if the signature was .then(error, result)?

It's very easy to mess up multi parameter APIs...objects are much more flexible. There is the possibility of key collision, but it's worth noting that js "classes" also have key collision (constructor) and objects themselves have key collision (prototype). We can't have "no" key collision, so how much do we want to have?

ericelliott commented 8 years ago

Which idea offers more freedom to stamp authors?

You're looking at an edge case (author wants to use "instance" or "stamp" as an option), and suggesting that we eliminate what may in the future be common case (author wants to utilize more than a single argument for their stamp) in order to enable the edge case.

The work-around if the author wants something like "instance" is to pick a non-reserved key to represent the "instance". This is what JS authors have been doing for years when they name a var args instead of arguments, because arguments was already bound inside the function. I don't think any developer thinks this is an unreasonable compromise. We also understand that we can't call a var Object or Array because those are built-ins.

What you're suggesting is a drastic measure (dictate an entire argument) to fix a problem that is already very small, and already has a simple workaround.

I realize that currently we don't offer any additional arguments, but I'd like the option of changing that in the future if users decide it's something they need.

koresar commented 8 years ago

@JosephClay your single argument proposal does not comply the Eric's comment above. He proposes to have multiple parameters to the stamp function.

But I hear your argument about constructor and prototype and the rest. Good point.

koresar commented 8 years ago

@ericelliott I hear your argument about the edge case, I'd agree it is. Also, I tend to agree that two arguments is somewhat worse than a single options object.

Could you please answer my two questions above? I must know answers now, and you need to add them to the specification text.

Also, I have to ask you to ignore my question less. That'd be very helpful in our future discussions. You tend to avoid straight questions lately. Probably you are very busy with your work.

ericelliott commented 8 years ago

@JosephClay your single argument proposal does not comply the Eric's comment above. He proposes to have multiple parameters to the stamp function.

I propose that we stick to a single argument for the parameters that we need in order to provide flexibility to stamp authors and users of the stamp specification in the future.

If we use more than one argument, we'll be stuck with the decision going forward. I made the wrong decision in this regard when I created the original Stampit API and decided that the first argument would be properties to assign to the object. That's a mistake I don't want to repeat.

koresar commented 8 years ago

That's a mistake I don't want to repeat.

:+1: I do agree. Let's stick with a single ergument. Though, my two questions worry me much.

ericelliott commented 8 years ago

The answer to your two questions, according to the current spec should be something like this:

{ a: 1 }
{ [Function: Stamp]
  compose: 
   { [Function: composeMethod]
     methods: {},
     properties: {},
     deepProperties: {},
     initializers: [ [Function] ],
     staticProperties: {},
     propertyDescriptors: {},
     staticPropertyDescriptors: {},
     configuration: {} } }

Reason for the 1st one:

The whole point of the instance parameter is so that users can pass an existing instance of something into the stamp for mutation, so, we use the object passed in as the instance.

Reason for the 2nd one:

When we call the initializer, we ignore the options and simply pass the stamp in as the stamp parameter. In other words, no collision is created. We don't even have to reserve the stamp option. Let me remind you of the proposed options argument for the initializers:

{
  instance,
  stamp,
  options
}

In other words, the initializer call might look something like this:

initializer.call(obj, { instance: obj, stamp: Stamp, options });

I'm not sure what you're worried about. Am I missing something?

ericelliott commented 8 years ago

If you're satisfied, please close. =)

koresar commented 8 years ago

I do not like the new "options" property we have. It complicates passing data from user to initializers.

let MyStamp = compose({ init({ options }) => { // <- this is Options inside Options
  request(options.requestOptions); // <- this is requestOptions inside Options inside Options
});

MyStamp({ requestOptions });

Options inside Options is a code smell. In my example there are "requestOptions inside Options inside Options" which is even worse.

I am strongly against that third argument.

From the discussion above we all are okay having two reserved properties. Let's drop "options" property from the intializer's options object.

koresar commented 8 years ago
let MyStamp = compose({ init({ requestOptions }) => { // <- this is Options
  request(requestOptions); // <- this is requestOptions inside Options
});

MyStamp({ requestOptions });
ericelliott commented 8 years ago

Point taken. I've been concentrating on the stamp parameters so far, but as far as I'm concerned, we can pass anything we want to init functions. Whatever is the most convenient:

// Note: simplified - no other stamp to compose with.
// Shouldn't these util functions return stamps?
let MyStamp = init( ({ requestOptions }, instance, stamp) => {
  request(requestOptions);
});

MyStamp({ requestOptions }); // this is the most common case, and doesn't change.
koresar commented 8 years ago

:+1:

koresar commented 8 years ago

Also, we might want to consider removing the second parameter:

let MyStamp = init( ({ requestOptions }, instance, stamp) => {

It is already part of the first argument:

let MyStamp = init( ({ requestOptions, instance }, stamp) => {

What do you think?

ericelliott commented 8 years ago

I think, to avoid conflating the options argument passed into the stamp with the init function parameters, maybe this would be better...

let MyStamp = init( ({ requestOptions }, { instance, stamp }) => {

It also provides a clear extensibility point in case we want to add arguments down the line...

let MyStamp = init( ({ requestOptions }, { instance, stamp, arguments }) => {

As we've discussed many times in the past, I think stamp arguments are an anti-pattern, but I thought I'd never allow it into Stampit, and then users demanded it, so I caved. =)

And maybe sometime down the line we'll think of other neat things we could pass into the init functions... I most definitely would prefer we handle that extensibility with an extensible object than a bunch of arguments...

ericelliott commented 8 years ago

Keep in mind, the stamp instance argument is not necessarily the same object passed into the initializer. For one thing, the argument is optional. For another thing, instances can be replaced by stamps. ;)

koresar commented 8 years ago

That is awfully beautiful idea! Do you want me to create a PR for that?

ericelliott commented 8 years ago

Sure, that would be great. =)

ericelliott commented 8 years ago

Oops.. accidentally self-merged this function signature update in the README:

(options, { instance, stamp }) => instance
koresar commented 8 years ago

I'm not yet closing this issue as a reminder for myself to create a PR. Although, this discussion should be considered successfully finished.

ericelliott commented 8 years ago

Well, before you dump much time into it... I'm conducting an exercise for myself right now -- creating a full implementation of the spec as it is now (including this change) with a full, descriptive unit test suite. Nowhere near complete, currently, but you can monitor my progress in the outstanding PR.

I'm doing this because I'm presenting the spec & example implementation at the WebDirections Conference in Sydney, Australia in October.

If you also want to work on an implementation, that's cool (maybe in the Stampit 3.0 branch?), but I need to do all of this one I'm working on from the ground up so that all of the design implications can really sink in. Does that sound like a fair deal?

koresar commented 8 years ago

Complicated wording. Pardon my English skills. What deal?

ericelliott commented 8 years ago

I work on the example code for the stamp spec, you work in parallel on the implementation for Stampit 3.0?

koresar commented 8 years ago

Sounds about right. But we should start form tests regardless.

ericelliott commented 8 years ago

Right now I'm working on one test at a time:

  1. Write the test
  2. Watch it fail
  3. Implement feature
  4. Watch it pass

The thing about TDD is that the tests don't just validate the code. The code also validates the tests. In other words, if you write a test suite with no implementation, there's no way to know whether or not the tests are valid, because they haven't been tested against real code.

koresar commented 8 years ago

I believe we can close this issue now. The README is up to date.