search

stampit-org / stamp

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

Differences in behaviour between packages `@stamp/it` and `stampit` when using accessor in props #82

Closed sambauers closed 1 year ago

sambauers commented 3 years ago

I am seeing some differences in results when implementing stamps using these packages. This is causing me some confusion and even concern about the "right way" to do things.

It appears that the behaviour of props differs between these packages when creating a new instance of a stamp which has accessors in props.

I'll provide an example in code:

// import stampit from '@stamp/it' // works
import stampit from 'stampit' // fails

// storing the value in global scope is bad - but that's not the point
let storedValueOne: string | undefined
const HasStringAccessorOne = stampit({
  props: {
    get one(): string | undefined {
      return storedValueOne
    },

    set one(value: string | undefined) {
      if (!['undefined', 'string'].includes(typeof value)) return
      storedValueOne = value
    },
  },

  init(opts) {
    this.one = opts.one
  },
})

const HasStringAccessorTwo = stampit({
  props: {
    two: undefined as string | undefined
  },

  init(opts) {
    this.two = opts.two
  },
})

const TestAccessors = stampit(
  HasStringAccessorOne,
  HasStringAccessorTwo,
  { name: 'TestAccessors' },
)

const instance1 = TestAccessors({ one: 'mango', two: 'apple'})
console.log('=== instance1 instantiated ===')
console.log()

// check instance1
console.log('instance1.one (accessor)')
console.log(' - expected: mango')
console.log(` -      got: ${instance1.one}`)
console.log(` -   result: ${instance1.one === 'mango' ? '🟢 pass' : '🔴 fail'}`)
console.log()
console.log('instance1.two (value)')
console.log(' - expected: apple')
console.log(` -      got: ${instance1.two}`)
console.log(` -   result: ${instance1.two === 'apple' ? '🟢 pass' : '🔴 fail'}`)
console.log()

const instance2 = TestAccessors({ one: 'banana', two: 'pear'})
console.log('=== instance2 instantiated ===')
console.log()

// check instance2
console.log('instance2.one (accessor)')
console.log(' - expected: banana')
console.log(` -      got: ${instance2.one}`)
console.log(` -   result: ${instance2.one === 'banana' ? '🟢 pass' : '🔴 fail'}`)
console.log()
console.log('instance2.two (value)')
console.log(' - expected: pear')
console.log(` -      got: ${instance2.two}`)
console.log(` -   result: ${instance2.two === 'pear' ? '🟢 pass' : '🔴 fail'}`)
console.log()

// check instance1 again
console.log('instance1.one (accessor)')
console.log(' - expected: mango')
console.log(` -      got: ${instance1.one}`)
console.log(` -   result: ${instance1.one === 'mango' ? '🟢 pass' : '🔴 fail'}`)
console.log()
console.log('instance1.two (value)')
console.log(' - expected: apple')
console.log(` -      got: ${instance1.two}`)
console.log(` -   result: ${instance1.two === 'apple' ? '🟢 pass' : '🔴 fail'}`)
console.log()

Result when using import stampit from '@stamp/it:

=== instance1 instantiated ===

instance1.one (accessor)
 - expected: mango
 -      got: mango
 -   result: 🟢 pass

instance1.two (value)
 - expected: apple
 -      got: apple
 -   result: 🟢 pass

=== instance2 instantiated ===

instance2.one (accessor)
 - expected: banana
 -      got: banana
 -   result: 🟢 pass

instance2.two (value)
 - expected: pear
 -      got: pear
 -   result: 🟢 pass

instance1.one (accessor)
 - expected: mango
 -      got: mango
 -   result: 🟢 pass

instance1.two (value)
 - expected: apple
 -      got: apple
 -   result: 🟢 pass

Result when using import stampit from 'stampit':

=== instance1 instantiated ===

instance1.one (accessor)
 - expected: mango
 -      got: mango
 -   result: 🟢 pass

instance1.two (value)
 - expected: apple
 -      got: apple
 -   result: 🟢 pass

=== instance2 instantiated ===

instance2.one (accessor)
 - expected: banana
 -      got: banana
 -   result: 🟢 pass

instance2.two (value)
 - expected: pear
 -      got: pear
 -   result: 🟢 pass

instance1.one (accessor)
 - expected: mango
 -      got: banana
 -   result: 🔴 fail

instance1.two (value)
 - expected: apple
 -      got: apple
 -   result: 🟢 pass

So it seems that stampit package deals with merging of accessors on props in a different way to @stamp/it, and I'm not sure which is correct. My expectation (as per the code above) would be that they should behave like standard props and not be copied into the new instance.

Note: you can ignore that I placed let storedValueOne in the global scope. This behaviour persists when the accessor stamp is built using a function. The only way to work around this behaviour is to place the stored value in another prop like _one, or by using a weak map, but the point here is the divergence in behaviour between the packages.

Here is some code using a "stamp builder" which encloses the stored value, which still fails when using stampit package:

import stampit from 'stampit'

const hasStringAccessor = (name: string) => {
  let storedValue: string | undefined

  return stampit({
    props: {
      get [name](): string | undefined {
        return storedValue
      },

      set [name](value: string | undefined) {
        if (!['undefined', 'string'].includes(typeof value)) return
        storedValue = value
      },
    },

    init(opts) {
      this[name] = opts[name]
    },
  })
}

const TestAccessors = stampit(
  hasStringAccessor('one'),
  { name: 'TestAccessors' },
)

const instance1 = TestAccessors({ one: 'mango'})
console.log('=== instance1 instantiated ===')
console.log()

// check instance1
console.log('instance1.one (accessor)')
console.log(' - expected: mango')
console.log(` -      got: ${instance1.one}`)
console.log(` -   result: ${instance1.one === 'mango' ? '🟢 pass' : '🔴 fail'}`)
console.log()

const instance2 = TestAccessors({ one: 'banana'})
console.log('=== instance2 instantiated ===')
console.log()

// check instance2
console.log('instance2.one (accessor)')
console.log(' - expected: banana')
console.log(` -      got: ${instance2.one}`)
console.log(` -   result: ${instance2.one === 'banana' ? '🟢 pass' : '🔴 fail'}`)
console.log()

// check instance1 again
console.log('instance1.one (accessor)')
console.log(' - expected: mango')
console.log(` -      got: ${instance1.one}`)
console.log(` -   result: ${instance1.one === 'mango' ? '🟢 pass' : '🔴 fail'}`)
console.log()
koresar commented 3 years ago

Hello @sambauers Thanks for rising this issue.

What is called "accessor" in your message is what we call "getters".

Getters and setters support is the most recent feature of stamps. https://github.com/stampit-org/stamp-specification/releases

The stampit does support getters out of the box. The @stamp/it does not yet. Sorry about that!

@PopGoesTheWza the root cause in the difference of packages is that migration of @stamp/* to TS was never completed. Hence, we didn't implement the getters in @stamp/it.

What's the stage of TS migration? Can we publish the new version of everything?

koresar commented 3 years ago

@sambauers There is a bug in your test. I've pulled it out to this gist: https://gist.github.com/koresar/21bd323148b2645d1d91601d8e2a8361

You are creating only one closure in the beginning of the test. And then reusing the closured storedValue for two stamp instances. This is the same as using global scope, just more localised. Here is the code for you to think of:

function createClosure() {
  var storedValue = Math.random();
  return () => storedValue;
}

const hasStringAccessorResult = createClosure();

let instance1 = hasStringAccessorResult();
let instance2 = hasStringAccessorResult();
console.log(` -   result: ${instance1 === instance2 ? '🟢 pass' : '🔴 fail'}`)
// This should pass, because the `Math.ramdom()` was called only once.

What you are looking to do is probably this:

  const HasStringAccessor = stampit({
    deepConf: {
      protectedProperties: []
    },
    statics: {
      protectProperty(...names) {
        return this.deepConf({ protectedProperties: names });
      }
    },
    init(keyValueMap, { stamp }) {
      const map = new Map(stamp.compose.deepConfiguration.protectedProperties.map(name => [name, undefined]));
      for (const [key, value] of Object.entries(keyValueMap)) {
        if (map.has(key)) map.set(key, value)
      }
      for (const name of map.keys()) {
        Object.defineProperty(this, name, {
          get () { return map.get(name); },
          set (value) {
            if (!["undefined", "string"].includes(typeof value)) return;
            map.set(name, value);
          }
        });
      }
    }
  });

And usage:

  const TestAccessors = stampit(
    HasStringAccessor.protectProperty("one", "two"),
    { name: "TestAccessors" }
  );

  const instance1 = TestAccessors({ one: "mango", two: "2" });
  instance1.one = {BadBanana: true};
  t.equal(instance1.one, "mango");
  instance1.two = ["bad array"];
  t.equal(instance1.two, "2");

  const instance2 = TestAccessors({ one: "banana", two: "222" });
  t.equal(instance2.one, "banana");
  t.equal(instance2.two, "222");

// check instance1 again
  t.equal(instance1.one, "mango");
  t.equal(instance1.two, "2");

  instance1.one = "1"
  t.equal(instance1.one, "1");
  instance2.one = "111"
  t.equal(instance2.one, "111");
sambauers commented 3 years ago

Right, so at this point I'm quite confused. I'm attempting to transition from OOP classes. So I just want to know what is the equivalent of a class property (the above discovery was part of my attempts to work that out). In an OOP class I can do:

class TestAccessors {
  public _one: string | undefined // can be private too - doesn't matter

  constructor(opts?: Record<string, string | undefined> = {}) {
    this.one = opts.one
  }

  get one(): string | undefined {
    return this._one
  }

  set one(value) {
    if (!['undefined', 'string'].includes(typeof value)) return
    this._one = value
  }
}

const instance1 = new TestAccessors({ one: 'mango' })
console.log(Object.assign({}, instance1)) // { _one: 'mango' } as expected

const instance2 = new TestAccessors({ one: 'banana' })
console.log(Object.assign({}, instance2)) // { _one: 'banana' } as expected
console.log(Object.assign({}, instance1)) // still { _one: 'mango' } as expected

How do I get the same behaviour as shown here (localisation of property public _one) with stamps? I know it is possible with weak maps and symbol keyed properties and the like but that all seems pretty hacky. Is the answer just to initialise values on properties in init? That will still maintain a reference right? So updating one stamp instance's prop will update all, right?

Or do I just use deepProps?

And despite reading everything, I still can't work out the utility of conf/deepConf.

koresar commented 3 years ago

How do I get

Mate I wrote you the code how to get it. :)

koresar commented 3 years ago

I don't quite get: do you want to hide _one or you don't? Can you please write five line unit test of what you are trying to achieve?

(Asking 5 questions one after another does not help.)

koresar commented 3 years ago

About conf and deepConf. These are just arbitrary metadata. It is not used by stampit or stamps. It just exists to hold anything you want for whatever need you have.

I typically use it to collect information about how a stamp should behave. (Like, which properties to protect.)

koresar commented 3 years ago

I wrote an extended version of the stamp above. It can protect not only strings, but also any type of data.

Usage:

  const _ = require("lodash");

  const TestAccessors = stampit(
    HasProtectedAccessor.protectProperty({
      fullName: v => v === undefined || _.isString(v),
      address: _.isArray,
    }),
    { name: "TestAccessors" }
  );

  const instance1 = TestAccessors({ fullName: "John Johnson", address: [] });
  instance1.fullName = 123.123; // WILL NOT WORK! Still "John Johnson"
  instance1.fullName = undefined; // ok
  instance1.address = 123.123; // WILL NOT WORK! Still []
  instance1.address = undefined; // WILL NOT WORK! Still []
  instance1.address = ["123 Acme St"]; // ok

Here is the full source code (also in this gist):


  const HasProtectedAccessor = stampit({
    // stamp configuration
    deepConf: {
      // collect all the property names to protect
      protectedProperties: {}
    },
    statics: {
      // API of the stamp
      protectProperty(names) {
        // will deep merge the base objects with `names`
        return this.deepConf({ protectedProperties: names });
      }
    },
    init(keyValueMap, { stamp }) {
      // The properties to protect
      let { protectedProperties } = stamp.compose.deepConfiguration || {};
      // The closured map of protector functions
      const protectors = new Map(Object.entries(protectedProperties)
        .filter(([key, value]) => typeof value === "function"));
      // The closured map of protected values
      const map = new Map();
      for (const key of protectors.keys()) map.set(key, undefined);

      for (const name of map.keys()) {
        // Using JS getters and setters to protect properties
        Object.defineProperty(this, name, {
          get () { return map.get(name); },
          set (value) {
            if (protectors.get(name)(value, name))
              map.set(name, value);
          }
        });
      }

      // Initialise this object instance with the configured values
      for (const [key, value] of Object.entries(keyValueMap)) {
        if (map.has(key)) this[key] = value;
      }
    }
  });

  const _ = require("lodash");
  const TestAccessors = stampit(
    HasProtectedAccessor.protectProperty({
      fullName: v => v === undefined || _.isString(v),
      address: _.isArray,
    }),
    { name: "TestAccessors" }
  );

  const instance1 = TestAccessors({ fullName: "John Johnson", address: [] });
  t.strictEqual(instance1.fullName, "John Johnson");
  t.deepEqual(instance1.address, []);

  const instance2 = TestAccessors({ fullName: ["array"], address: "oops" }); // WILL NOT SET
  t.strictEqual(instance2.fullName, undefined);
  t.strictEqual(instance2.address, undefined);

  instance2.fullName = function () {}; // WILL NOT SET
  t.strictEqual(instance2.fullName, undefined);
  instance2.address = { NotAnArray: 111 }; // WILL NOT SET
  t.strictEqual(instance2.address, undefined);

  instance2.fullName = "Andrew Anderson";
  t.strictEqual(instance2.fullName, "Andrew Anderson");
  instance2.address = ["111 Acme St", "London"];
  t.deepEqual(instance2.address, ["111 Acme St", "London"]);

  instance2.fullName = undefined;
  t.strictEqual(instance2.fullName, undefined);
  instance2.address = undefined; // WILL NOT SET
  t.deepEqual(instance2.address, ["111 Acme St", "London"]);
koresar commented 3 years ago

I got a little excited and blogged about it :) https://medium.com/@koresar/fws-s2e3-generic-smart-setters-ec9cf2e0e997

Thank you Sam for the inspiration.

sambauers commented 3 years ago

Thank you for taking the time to write a comprehensive solution. I had hoped there was a built-in way to solve this that I hadn't been able to see.

Sorry about my rapid-fire questions.

In the context of this issue (the different behaviour between packages) I didn't care if the property in the classic OOP class or it's stamp equivalent was public or private. I just wanted each instance of my class or stamp to start with default values, and not have the same reference to the stored value. I understand what you mean about the way I was trying to build a stamp effectively providing the same reference to both stamp instances. I'm inclined to either use props or deepProps with symbols as keys to store those values now, or a weak map on this to make them truly hidden from sight.

Reading this blog post again - https://medium.com/@koresar/fun-with-stamps-episode-25-getters-and-setters-built-in-support-54ec74f5829b - I notice you are putting getters and setters on methods. Is this preferable to props? - e.g.

const buildStamp = (name) => {
  const propName = `_${name}` // or Symbol(name) to stop direct access

  return stampit({
    props: {
      [propName]: undefined
    },
    methods: {
      get [name]() {
        return this[propName]
      },
      set [name](value) {
        this[propName] = value
      }
    }
  })
}
koresar commented 3 years ago

Hello. Good to see you on track! Sorry for the late response. Busy as hell.

To automatically copy data (as in JS primitives) you should use deepProps (and even props if you don't have arrays or objects in there). It would copy data for you. Also, it is always a good idea to have an initializer which copies data. Somehow, when people coming from classic classes, they struggle understanding the concept of having indefinite number of object initialisers. :)

BTW, I find confusing these two sentences:

I didn't care if the property ... was public or private

and

to make them truly hidden from sight

Sounds like you did care. :))) But that doesn't really matter.

Answering your question about putting getters and setters to methods.

Having getters and setters in props allows them to have closured private data (within the initialiser). But you copy each getter/setter per every new object instance.

Having getters and setters in methods though is more memory friendly, because methods is actually your object's prototype. Meaning, there will be only one getter/setter per multiple object instances created from the stamp.