stampit-org / stamp

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

Add whitelist behavior to "collision" package #17

Closed hoschi closed 7 years ago

hoschi commented 7 years ago

https://github.com/stampit-org/stamp/tree/c0adb79765cf059f175113b1d8679b5aae8bf2d1/packages/collision

At the moment there is no way to use collisionProtectAnyMethod as default and then allow a method to be overridden as special case. For me it makes more sense to encourage collision free stamps, but be able to use defer or replace a method when it makes sense.

koresar commented 7 years ago

Thank you for the idea. Do you have an idea of preferred API for that?

hoschi commented 7 years ago

Your welcome. As forbid is the current blacklist behavior the opposite word should do it. I'm not a native speaker, but allow makes sense to me.

Example

const AllowCacheCollision = Collision
  .collisionProtectAnyMethod()
  .collisionSetup({allow: ['addToCache', 'removeFromCache', 'isCached']});
koresar commented 7 years ago

Lgtm. Should be trivial to implement. Will do in a 24 h.

koresar commented 7 years ago

Published. Here is the change.

hoschi commented 7 years ago

LGTM, thanks

hoschi commented 7 years ago

@koresar I don't get it :(

DefaultStamp.js:

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

let ProtectMethods = Collision.collisionProtectAnyMethod();

export let DefaultStamp = stampit().compose(ProtectMethods)

export default DefaultStamp;

DefaultStamp.test.js

import test from 'ava';
import DefaultStamp from './DefaultStamp'

test('prevents collision by default', t => {
    let First = DefaultStamp.methods({
        doIt () {
            return 'foo';
        },
    });
    let first = First.create();
    t.truthy(first.doIt() === 'foo')

    let Second = First.methods({
        doIt () {
            return 'bar'
        }
    })
    let second = Second.create();
    t.truthy(second.doIt() === 'bar')
})

Test passes, but should't throw the line let Second = First.methods(...) because it overrides the doIt method of First?

version:

npm ls | agi stamp  
├─┬ @stamp/collision@0.2.0
│ ├── @stamp/core@0.1.2
│ └── @stamp/is@0.1.2
├── @stamp/compose@0.1.2
│ │ │ └── time-stamp@1.0.1
├── stampit@3.1.2
koresar commented 7 years ago

Thank you for reporting.

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

Looks like that stampit is incompatible with the Collision stamp. I'll take a look tomorrow (its 11 pm atm).


@hoschi I have a very important question to ask. Could you please answer it.

You are not using the @stamp/compose from the examples, but using the stampit. What are the features you need the most so that you moved from stampit to the upcoming compatible module @stamp/it ?

hoschi commented 7 years ago

Thanks for looking into it ;)

What are the features you need the most so that you moved from stampit to the upcoming compatible module @stamp/it ?

I'm pretty happy with the current stampit module, so I guess the same API and features for @stamp/it would be what I need for a switch. Did I get your question right?

koresar commented 7 years ago

Thanks. It's good!

hoschi commented 7 years ago

@koresar is this ticket now blocked by this new @stamp/it module thingy?

koresar commented 7 years ago

Hello. The stampit and some @stamp/ some modules appears to be incompatible. But the @stamp/it module is coming! Sorry about that. Should be ready this week.

On Tue, 4 Apr 2017, 00:56 Stefan Gojan notifications@github.com wrote:

@koresar https://github.com/koresar is this ticket now blocked by this new @stamp/it module thingy?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stampit-org/stamp/issues/17#issuecomment-291168455, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjCL_KqvGK-0a_FN7skvsXWuG4smwO_ks5rsQiDgaJpZM4MPgzl .

hoschi commented 7 years ago

Ok, thanks for the update

Vasyl Boroviak notifications@github.com schrieb am Mo., 3. Apr. 2017, 22:11:

Hello. The stampit and some @stamp/ some modules appears to be incompatible. But the @stamp/it module is coming! Sorry about that. Should be ready this week.

On Tue, 4 Apr 2017, 00:56 Stefan Gojan notifications@github.com wrote:

@koresar https://github.com/koresar is this ticket now blocked by this new @stamp/it module thingy?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stampit-org/stamp/issues/17#issuecomment-291168455, or mute the thread < https://github.com/notifications/unsubscribe-auth/ABjCL_KqvGK-0a_FN7skvsXWuG4smwO_ks5rsQiDgaJpZM4MPgzl

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stampit-org/stamp/issues/17#issuecomment-291259409, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJ9OOcUy8KbQ-nhDuyKzJwKFaWRZYI0ks5rsVJ3gaJpZM4MPgzl .

koresar commented 7 years ago

@hoschi the @stamp/it was just published. Currently I'm writing a blog post about it.

koresar commented 7 years ago

The blogpost just went live too: https://medium.com/@koresar/episode-14-new-stamp-it-as-a-replacement-of-stampit-191ef0f4c53e

hoschi commented 7 years ago

This works now, thank you!