stampit-org / stamp

Stamps - better OOP model
https://stampit.js.org
MIT License
25 stars 3 forks source link

"Collision" stamp detects collisions when the colliding method comes from the same stamp #32

Closed hoschi closed 7 years ago

hoschi commented 7 years ago

I try to use the new stamp/it module which works got so far, but I found another problem with collision detection :(

DefaultStamp, used for every new stamp which doesn't compose with another already existing stamp:

import stampit from '@stamp/it'
import compose from '@stamp/compose'
import Collision from '@stamp/collision';

let ProtectMethods = Collision.collisionProtectAnyMethod();

let DefaultStamp = compose(stampit(), ProtectMethods)

export default DefaultStamp;

test

test('using the same stamp in other stamps is ok', t => {
    let Named = DefaultStamp.deepProps({
        name:undefined
    }).methods({
        getName() {
            return this.name;
        },
    })
    let base = Named.deepProps({
        name:'base'
    }).create();
    t.truthy(base.getName() === 'base')

    let Robot = Named.methods({
        getRobotLaws() {
            return 'be nice'
        }
    })

    let Animal = Named.methods({
        getAnimalLaws() {
            return 'king of jungle'
        }
    })

    let Robotimal = Robot.compose(Animal);
})

which fails with:

DefaultStamp › using the same stamp in other stamps is ok
  Error: Collision of method `getName` is forbidden
    module.exports.compose.composers (node_modules/@stamp/collision/index.js:128:19)
    Function.compose (node_modules/@stamp/compose/index.js:153:27)
    Function.stampit (node_modules/@stamp/it/index.js:83:18)
    Function._compose [as compose] (node_modules/@stamp/compose/index.js:66:34)
    Test.t [as fn] (src/services/DefaultStamp.test.js:83:27)

I see why it happens, in the last line I use two stamps which both have getName. I can solve this using Named stamp in Robotimal and not the other stamps.

My Problem here is that at the moment when I write Robot and not thinking about Robotimal I need to use Named so Robot is fully functional. Months later when I realize I need a Robotimal I need to refactor all stamps I use for the new stamp.

Is it possible to identity check the stamp getName came from and see that it is the same stamp (Named) and don't declare it as collision? Because it doesn't override the existing behavior at all.

koresar commented 7 years ago

That sucks! Sorry for the bug. Please, allow me 12 hours to fix and publish it.

The fix should be trivial. Each method is stored as a reference. Meaning that:

const Base = Collision.collisionProtectAnyMethod().methods({ getName() { return 'name'; } });

const Stamp1 = Base.props().methods();
const Stamp2 = Base.props().methods();

const Final = stampit(Stamp1, Stamp2); // THIS MUST NOT THROW

So: Stamp1.compose.methods.getName should be === to the Stamp2.compose.methods.getName.

This line should be fixed: https://github.com/stampit-org/stamp/blob/ce12385169ccd70d1b021421d974805143eb6c66/packages/collision/index.js#L41

hoschi commented 7 years ago

The fix should be trivial.

niiiice, I hoped to hear that :+1:

Please, allow me 12 hours to fix and publish it.

Awesome! I would then test it on Monday and report back fast.

koresar commented 7 years ago

Published as @stamp/collision@0.2.5

hoschi commented 7 years ago

Thanks, I think I can do the update this week

hoschi commented 7 years ago

works! :1st_place_medal: