stampit-org / stamp-specification

The Stamp Specification: Composables
434 stars 16 forks source link

Code review of stamp based project #107

Closed danielkcz closed 7 years ago

danielkcz commented 8 years ago

Based on discussion with @ericelliott in #70.

As I mentioned before, I've been using stamps for years -- since before my book was published, and I have never needed dedupe. I would suggest to you that perhaps if you think you need it, you're making your stamps too big -- piecing together things that maybe shouldn't be pieced together in the first place?

Maybe it's better to make smaller, independent stamps rather than merging together some large, complex stamp.

I don't suppose you have some small open code project using stamps more in real life? So far it's more about theory and everyone's interpretation on how to use stamps. It would be really nice to see some more professional approach.

I have this little project I've been writing lately and I've been learning stamps with it. Almost everything is stamp there. It's lacking documentation quite a lot yet, but it's almost 100% test covered. I am not expecting full code review, but some pointers about what do you consider overuse of stamps perhaps?

ericelliott commented 8 years ago

This is a bit old, but here's an OSS example in qconf.

danielkcz commented 8 years ago

Thanks, but that doesn't help much as there is very little of stamps, not much toward the power of composability.

koresar commented 8 years ago

I see nothing wrong with the way the stamps are used in the project. It's just one of the possible ways.

ericelliott commented 8 years ago

Thanks, but that doesn't help much as there is very little of stamps, not much toward the power of composability.

Yep. However, this is a very typical use-case for me. I usually don't compose more than 2 or 3 stamps together.

ericelliott commented 8 years ago

@FredyC I didn't go very deep, but stuff like this is OK:

import stampit from 'stampit';

import Logger from './core/Logger';

import SubjectList from './SubjectList';
import TreeList from './TreeList';

const Runner = stampit({
    methods: { tick },
}).compose(SubjectList, TreeList, Logger);

function tick() {
    for (const subject of this.listSubjects()) {
        const tree = this.getTree(subject.getTreeId());
        this.debug('tick', '%s ticks %s', tree, subject);
        tree.tick(subject);
    }
}

export default Runner;

As long as SubjectList, TreeList, and Logger don't know about each other, it's fine for Runner to know about the stuff it composes. e.g., this.getTree is defined in TreeList, which is explicitly imported: import TreeList from './TreeList';

This is a good example of how I use stamp composition.

That said, if Logger knew about TreeList props, that would be an anti-pattern.

danielkcz commented 8 years ago

Thanks, yea it seemed only logical to me to do it like this. I certainly did not like assumption there is some method getTree that gets composed elsewhere. But at beginning I was certainly confused what will actually happen with the multiple composition of the same thing and at some point I had real issue where initializer executed caused some problems.

@boneskull Perhaps you might be interested in checking this out as you said you don't explicitly compose dependencies.

koresar commented 7 years ago

Seem finished. Closing. Feel free to reopen though.