Multiple use of on('init') might cause inconsistencies

[yonjah]

I don't have a concrete issue, but I was looking for some other stuff and noticed this code - https://github.com/DockYard/ember-validations/blob/master/addon/validators/base.js#L16

The base object is calling multiple functions using on init. Some of this functions are relying on other function on the on init process to finish (like pushConditionalDependentValidationKeys that has to run before addObserversForDependentValidationKeys ). But since the execution order of on init is not guaranteed this create a very fragile code.

I made this pull request to fix this issue. I couldn't get the automatic test to work so a more careful examination of this fix might be necessary .

P.S - On another look I think the pushDependentValidationKeyToModel is not really doing any thing so I removed it. It treated model.dependentValidationKeys as an object even it was an array, and it tries to add this property to it even though it was add in the main init function

[ghost]

@yonjah Can you rebase on master, now that we have fixed the test suite?

[bcardarella]

on('init') was considered a best practice before ES6 gave use the spread operator. It is no longer the case: https://dockyard.com/blog/2014/04/28/dont-override-init

[yonjah]

@bcardarella I'm sure there was a good reason to use on('init') when this code was first created but I can't see it now. Even if its best practice having multiple on('init') calls in your object which are dependent on each other is fragile and bad practice in any circumstance.

@martndemus I'll try to pull in the changes from master and rebase

[yonjah]

How are you guys with breaking changes to the API ? I saw the failing tests and they are do to some tests adding parameters to the dependentValidationKeys after init super has finished.

There are two ways around it -

1. having a dedicated method to do that ( That's what I did in this commit)
2. having one method that will be called "on('init')" and will be in charge of syncing all this actions.

I personally prefer the first way but it is not how the API is currently designed.

Also there is a test that actually check if pushDependentValidationKeyToModel actually overloaded the array with the dependency parameter. I'm not sure why this code and test is there but if it is really necessary I might have to push it back in (but then I'll probably have to use the second way)

[bcardarella]

Technically the library is still in alpha, so if it is a valid change we can do it

[yonjah]

I reverted most of the changes only aggregating everything into a single on('init') method. There might be some benefits in changing that and moving away from on('init') use in the base class but this should probably be as part of a larger change that will consider the entire class API