sylvainpolletvillard / ObjectModel

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

Leftover Validation Errors preventing casting #98

Closed gotjoshua closed 6 years ago

gotjoshua commented 6 years ago

Wow this one's been driving me nuts! https://codepen.io/gotjoshua/pen/QVygdN

If i use a custom error collector with validate and it finds errors, they seem to be "leftover" after i fix the obj and try to cast it!

gotjoshua commented 6 years ago

Can anyone confirm this behavior/bug? Any help with a workaround?

Am I missing something?

sylvainpolletvillard commented 6 years ago

Bug confirmed, fixed and released as v3.7.4

It gave me a hard time too, probably because I come back from the bar 😅

The bug was related to class extensions. When errors were consumed, I was resetting the error stack with model.errors = []. But doing this with an extended class will define the errors property for the class, and keep the original model.errors value for the prototype:

const SubOM = BaseClass.extend({ test: [Boolean] }).assert(o => o.test)
class SubClass extends SubOM { }
SubClass.validate({ test: false }, () => { });
console.log(SubClass.errors) // []
console.log(SubOM .errors) // [ { assertion error } ], not cleaned up properly

The trick was to mutate the array with model.errors.length = 0 to clear it without reassignment. Not very common, I learned something today. Thanks for the report !

gotjoshua commented 6 years ago

Nice one! 2AM post-bar bug fix....

I had my eye on that line 43, when I was trying to figure out what the hell was going on... but it seemed to be the right thing to do. How did you find/figure out that trick??? I thought/assumed array.length was read only!

sylvainpolletvillard commented 6 years ago

How did you find/figure out that trick???

I never reveal my secrets

https://stackoverflow.com/questions/1232040/how-do-i-empty-an-array-in-javascript

gotjoshua commented 6 years ago

Ha! : -D

So, after reviewing the link, I am curious why you chose to set the length instead of errors.splice(0) (or more explicitly errors.splice(0,errors.length). I see that performance wise it is identical: http://jsben.ch/hyj65 (ok, in FF slightly slower) and strikes me as more intuitive (still trying to wrap my head around why arr.length is a setter!) cheers...

sylvainpolletvillard commented 6 years ago

length is 50% faster than splice on another bench with 100k items: https://jsperf.com/array-clear-methods/3

gotjoshua commented 6 years ago

I find the historical changes on that site remarkable... pop() used to be way faster then at some point redefine took over - but in modern browsers i don't see any tests that indicate length being 50% faster... but actually i am more curious about your decision, was it purely speed based, or did you consider the other options with other thoughts?

sylvainpolletvillard commented 6 years ago

I got these results: image

Didn't think too much about it, length was short to write and apparently faster than splice. More importantly, it fixed the bug. Remember that it was 2AM, I shipped and went to bed