sylvainpolletvillard / ObjectModel

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

Can't update two properties at once, causing assertion issues #116

Closed esegal closed 4 years ago

esegal commented 5 years ago

I have an assertion that checks "end" is after "start". When I update my OM, I need to know the existing values to know which fields updates first.

I used Object.assign to update both fields at once but this fails too, as Object.assign internally iterates the keys.

Here's a demo of the issue: https://jsfiddle.net/psaqyouz/1/

Suggestion: to have ObjectModel.assign - to change a few values at once, and only than run assertions

sylvainpolletvillard commented 5 years ago

Hi,

Unfortunately your suggestion is not technically feasible. As you said, Object.assign does not change the fact that properties are internally changed one after the other, and assertions are ran every time a property changes. If objectmodel was "delaying" the validation step, to wait for other props to be changed as well for example, that would mean that there is a timespan during which the model instance is in an invalid state, which is something we don't want to happen.

You have other solutions though. You can do it with a 3-step assignment:

MyModel.prototype.updateRange = function(start, end){
  this.start = start > this.end ? this.end-1 : start;
  this.end = end;
  this.start = start
 }

let OM = new MyModel({start:1, end:2});

OM.updateRange(3,4)

or you can temporary disable the assertion during this specific operation:

MyModel.prototype.updateRange = function(start, end){   
  const disabledAssertions = MyModel.assertions;
  MyModel.assertions = [];
  this.start = start;
  MyModel.assertions = disabledAssertions;
  this.end = end;  
 }
gotjoshua commented 5 years ago

well, another alternative is to assign to an empty object and then cast like in this fiddle. basically to create an uncast object with all properties set and then cast all at once.

OM = Object.assign({}, OM, {start:3, end:4});

this of course creates a new object reference.

I agree with @sylvainpolletvillard that:

that would mean that there is a timespan during which the model instance is in an invalid state, which is something we don't want to happen.

I also agree with @esegal that object model could offer a simple "assign" or "update" function that would take care of this assign with delayed assertions. As, in this case there is no "timespan during which the model instance is in an invalid state" because we explicitly update multiple props together.

MyModel.prototype.update = function assignFirstThenAssert(newPropsObject){
  const disabledAssertions = this.constructor.assertions;
  this.constructor.assertions = [];
  Object.assign(this,newPropsObject);
  this.constructor.assertions = disabledAssertions;
}

This i managed to implement in a later version of the same fiddle, but could not manage to hack the update FX onto the ObjectModel.prototype itself... anyway, i imagine @sylvainpolletvillard will have an elegant way to do that if he decides that the idea is worth it.

sylvainpolletvillard commented 5 years ago

This looks like a very specific issue, something I never saw in 3 years of existence of this project, so I'm not convinced it is worth extending the API. Moreover, if such method would exist, I don't see a clear answer to what to disable and how long.

I think the last workaround you proposed, reassigning the parent object, is the most simple and elegant way to deal with this issue.

If this is not an acceptable solution, as @gotjoshua demonstrated, adding your own method is easily feasible in userland. Note that I generally want to avoid extending ObjectModel.prototype, since it is as dangerous as extending Object.prototype, it adds a new property to every object so there is a high risk of breaking existing code.

sylvainpolletvillard commented 4 years ago

It's been 2 monts, I think the issue has been answered. Closing this, if you want to further discuss feel free to reopen.