sylvainpolletvillard / ObjectModel

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

TypeError: 'set' on proxy: trap returned falsish for property [which is actually a method] #97

Closed gotjoshua closed 6 years ago

gotjoshua commented 6 years ago

In the midst of a BookingOM.test() call i get this: Bookings.js:48 Uncaught TypeError: 'set' on proxy: trap returned falsish for property 'update'

in BookingOM, 'update' is a method:

update = (changeSet) => {
    // console.log('Bkng:' + this.ID + ' with:' + Object.keys(changeSet).length);
    for (let eachProp in changeSet) {
      if (changeSet[eachProp] != undefined) {
        // console.log(eachProp + ': ' + changeSet[eachProp]);
        this[eachProp] = changeSet[eachProp];
      }
    }
  };

any help?

sylvainpolletvillard commented 6 years ago

Is it since v3.7.2 ? And could you show the entire code, how you add this update method to the BookingOM model ?

sylvainpolletvillard commented 6 years ago

I also assume you mean this[eachProp] instead of this.eachProp, even if it's unrelated

gotjoshua commented 6 years ago

it was first in 3.7.1, but now also in 3.7.2: seems to be in the new BookingOM within the test method: omerrorontest

full code of BookingOM:

class BookingOM extends BaseOM.extend({
  _id: [String],
  startDate: Date,
  endDate: Date,
  placeId: String, // required TODO check test data should be mongo _ids

  groupId: [MongoId], // mongo string ID
  statusId: [MongoId], // mongo string ID
  dailyFeeId: [MongoId], // mongo string ID (nested only in VMs)
  price: [Number], // stored as price in Euros - set via pulldown from PriceSets/Prices
  comments: [String], // comments text
  invoiceId: [MongoId],
  customcolor: [String], // color HEX string eg: #223344

  contactId: [CiviId], // CiviCRM Contact ID - Integer (transferred as String in JSON)
})
  .assert((o) => {
    return !BookingOM.checkBlockingStatus(o.statusId) || !!o.comments;
  }, 'Comments are Required if status is Blocking')
  .assert((o) => {
    return BookingOM.checkBlockingStatus(o.statusId) || !!o.groupId;
  }, 'Group is Required if status is not Blocking')
  .assert((o) => {
    return BookingOM.checkBlockingStatus(o.statusId) || !!o.dailyFeeId;
  }, 'Daily Fee is Required if status is not Blocking')
  .assert((o) => {
    return BookingOM.checkBlockingStatus(o.statusId) || !!o.statusId;
  }, 'Status is Required if status is not Blocking')
  .assert((o) => {
    return BookingOM.checkBlockingStatus(o.statusId) || !!o.contactId;
  }, 'Contact is Required  if status is not Blocking') {
  get isBlocking() {
    return BookingOM.checkBlockingStatus(this.statusId);
  }
  get ID() {
    return this._id;
  }
  update = (changeSet) => {
    // console.log('Bkng:' + this.ID + ' with:' + Object.keys(changeSet).length);
    for (let eachProp in changeSet) {
      if (changeSet[eachProp] != undefined) {
        // console.log(eachProp + ': ' + changeSet[eachProp]);
        this[eachProp] = changeSet[eachProp];
      }
    }
  };
  static checkBlockingStatus(statusId) {
    return Statuses.isBlockingStatus(statusId);
  }
  setDefaults(force) {
    let getDefFor = BookerData.Defaults.getFor;
    if (force) {
      this.groupId = getDefFor('groups');
      this.statusId = getDefFor('statuses');
      this.dailyFeeId = getDefFor('dailyfees');
    } else {
      this.groupId = this.groupId || getDefFor('groups');
      this.statusId = this.statusId || getDefFor('statuses');
      this.dailyFeeId = this.dailyFeeId || getDefFor('dailyfees');
    }
  }
  static setDefaultsOn(obj, force) {
    let getDefFor = BookerData.Defaults.getFor;
    if (force) {
      obj.groupId = getDefFor('groups');
      obj.statusId = getDefFor('statuses');
      obj.dailyFeeId = getDefFor('dailyfees');
    } else {
      obj.groupId = obj.groupId || getDefFor('groups');
      obj.statusId = obj.statusId || getDefFor('statuses');
      obj.dailyFeeId = obj.dailyFeeId || getDefFor('dailyfees');
      return obj;
    }
  }
}
sylvainpolletvillard commented 6 years ago

This syntax update = (changeSet) => is invalid for class method definition in ES2018, are you using a custom Babel plugin ?

Try this instead:

update(changeSet) {
    // console.log('Bkng:' + this.ID + ' with:' + Object.keys(changeSet).length);
    for (let eachProp in changeSet) {
      if (changeSet[eachProp] != undefined) {
        // console.log(eachProp + ': ' + changeSet[eachProp]);
        this[eachProp] = changeSet[eachProp];
      }
    }
  }

If you are using this Babel plugin, then the syntax you used implies you are assigning the update method as an own property of your class, same as this.update = () => ... in the class constructor.

gotjoshua commented 6 years ago

yes, the great thing about the syntax we are using is that it binds the function to the instance (without any ugly bind calls in the constructor). I was not aware of the babel magic that is responsible for that (but we are indeed using babel with Meteor).

With the syntax that you are suggesting, will i need to bind the methods manually? Eg:

constructor(){
  this.update=this.update.bind(this);
}

Maybe this question is showing my embarrassing level of basic js knowledge, but there are many examples using this syntax (especially for methods that are called via a callback - and are thus prone to annoying this dilemmas)

sylvainpolletvillard commented 6 years ago

It doesn't bind, no. You'll get an error if you do:

const { update } = myBookingOM
update(); // lose 'this' context

I need to see how Babel rewrites this code. Maybe this autobinding is responsible of the error

sylvainpolletvillard commented 6 years ago

I tried this code:

const BaseOM = ObjectModel({})
class BookingOM extends BaseOM.extend({
  _id: [String],
})
 {
  update = (changeSet) => {
    // console.log('Bkng:' + this.ID + ' with:' + Object.keys(changeSet).length);
    for (let eachProp in changeSet) {
      if (changeSet[eachProp] != undefined) {
        // console.log(eachProp + ': ' + changeSet[eachProp]);
        this[eachProp] = changeSet[eachProp];
      }
    }
  }
}

new BookingOM()

Here is the code output by Babel with babel-plugin-transform-class-properties:

const BaseOM = ObjectModel({});
class BookingOM extends BaseOM.extend({
  _id: [String]
}) {
  constructor(...args) {
    var _temp;

    return _temp = super(...args), this.update = changeSet => {
      for (let eachProp in changeSet) {
        if (changeSet[eachProp] != undefined) {
          this[eachProp] = changeSet[eachProp];
        }
      }
    }, _temp;
  }

}

new BookingOM();

No error triggered. I must be missing something. Could you try to make a reproducable example in a codepen or something ?

gotjoshua commented 6 years ago

Well, i'm not sure how to make it reproducible - and as this seems to be a detailed babel issue - i think its my problem not yours...

i worked around it by only running validate at that point (not test); not sure how this workaround will be impacted by the proposal for v4 ( #96 )...

sylvainpolletvillard commented 6 years ago

If you manage to get a simple reproducable test, leave it here, it might be a bug, even if it is an edge case