sylvainpolletvillard / ObjectModel

Strong Dynamically Typed Object Modeling for JavaScript
http://objectmodel.js.org
MIT License
467 stars 30 forks source link

Enumeration of read-only getters #91

Closed gotjoshua closed 6 years ago

gotjoshua commented 6 years ago

Why are props of an ObjectModel which are only accessible via getters, not automatically enumerable?

Example 1 (I expect gettableNestedInfo to enumerate):

let NestedOM = ObjectModel({nestedInfo:[String]});
class Mod extends ObjectModel({
  id: Number,
  nested: [NestedOM],
  gettableNestedInfo: [String],
  info: [String]
}){
  get gettableNestedInfo(){
    return this.nested.nestedInfo;
  }
}

https://codepen.io/gotjoshua/pen/qyzyWq

i could imagine that if there was a getter but no property in the model definition, then it would not be enumerable:

let NestedOM = ObjectModel({nestedInfo:[String]});
class Mod extends ObjectModel({
  id: Number,
  nested: [NestedOM],
  // gettableNestedInfo: [String],
  info: [String]
}){
  get gettableNestedInfo(){
    return this.nested.nestedInfo;
  }
}

As usual, I assume that you have put more thought into it than I, I just find it awkward to have to add all these bulky defineProperty commands:

Object.defineProperty(Mod.prototype, 'gettableNestedInfo', {
  enumerable: true,
});
sylvainpolletvillard commented 6 years ago

Except for private props, ObjectModel does not change the enumerability of object properties. In ES6, class getters/setters are not enumerable by default. You may be interested by this discussion: https://esdiscuss.org/topic/why-are-class-getters-setters-non-enumerable

gotjoshua commented 6 years ago

Hmmm... interested, but unsatisfied ; -)

Is there a convenient syntax for me to traverse the model and ensure that all defined props (that aren't private) are set as enumerable...

Or, is this a potential api option?

sylvainpolletvillard commented 6 years ago

No easy way that I know of, no. See https://stackoverflow.com/questions/34517538/setting-an-es6-class-getter-to-enumerable

Out of curiosity, why do you need all these properties to be enumerable ?

gotjoshua commented 6 years ago

I am passing the VM objects to fullcalendar, and the VM is exposing read-only typed getters for the props that FullCalendar is expecting. Internally, fullcalendar must be looping or assigning or somehow iterating in a way that works with enumeration but not without...

sylvainpolletvillard commented 6 years ago

I see. I did not find a simpler way than your current solution based on Object.defineProperty to set a property enumerable. I also think this is not the responsability of ObjectModel to add an API for that, as you would have the exact same problem without using the library.

gotjoshua commented 6 years ago

so....

I see that you are defining Props in helpers.js and defaulting enumerable to false:

define = (obj, key, value, enumerable = false) => {
    Object.defineProperty(obj, key, { value, enumerable, writable: true, configurable: true })
}

i made a for loop that will default all getters to be enumerable:

for (let eachProp of Object.keys(BookingVM.definition)) {
  console.log('of keys: ' + eachProp);
  let currentPropDef = Object.getOwnPropertyDescriptor(BookingVM.prototype, eachProp);
  if (!currentPropDef) continue;
  console.log(currentPropDef);
  currentPropDef['enumerable'] = true;
  Object.defineProperty(BookingVM.prototype, eachProp, currentPropDef);
}

Is there anyway that a property on the model could be set and take care this (or even similar to overriding the private convention "globally" on Model itself.

eg:

//globally:
Model.enumerableDefault=true;
//or specifically
BookingVM.enumerableDefault=true;

anyway the above for loop seems to do the trick, but i am curious about your opinions regarding the api proposal (wouldn't break anything as it would need to be consciously configured to change the current behavior)

gotjoshua commented 6 years ago

Well responsibility is a strong and formal word ; -) Yet I agree, totally not your/OM's responsibility...

but as you said elsewhere: "There is a compromise to find between simplicity vs convenience." perhaps there is a similar compromise to make between transparency and convenience...

I mean, it seems that you prefer for instances of OMs to behave as "transparently" identical to their raw js counterparts as possible ("you would have the exact same problem without using the library"). Yet, you also solve some shortcomings of js quite elegantly for us, your users. (eg _private and CONSTANT conventions)

From my perspective, enumerability default could be considered on the same level as "conventions" ...

sylvainpolletvillard commented 6 years ago

This internal define method is only used for some specific properties such as model constructor or assertions description. It's not used for your class getters or any other property.

Admittedly, private and constant props go beyond the basic features of dynamic type-checking. But at the same time, they are based on popular conventions among JS devs. Such a convention does not exist for enumerability, probably because the usecases are much less common. I could argue that this can be considered an issue for FullCalendar to not be able to use these non enumerable properties.

I think the best solution for you would be to declare a helper to make getters enumerable:

function setGettersEnumerable(o) {
    if (o != null) {
        let descriptors = Object.getOwnPropertyDescriptors(o)
        for (let prop in descriptors) {
            let descriptor = descriptors[prop]
            if (typeof descriptor.get === "function" && descriptor.enumerable === false) {
                Object.defineProperty(o, prop, { enumerable: true });
            }
        }
        setGettersEnumerable(Object.getPrototypeOf(o))
    }
}

usage:

constructor(){
    setGettersEnumerable(this);
}

or without constructor:

setGettersEnumerable(VM.prototype)

Note that setting every getter enumerable can have unexpected side effects. There are legit reasons why these properties are not enumerable in the first place.

gotjoshua commented 6 years ago

well, i thought about only setting the getters that are explicitly set in the OM definition (not any and all getters), but perhaps i need to rethink the strategy here... and as you said I can also approach FullCalendar dev(s) about the issue.

thanks again for your thorough help!