search

betsol / angular-input-modified

Angular.js module to detect and indicate input modifications
90 stars 35 forks source link

Consider decorating ngModel instead of overwriting it #33

Closed sscovil closed 7 years ago

sscovil commented 8 years ago

I was just looking through the source and noticed that you are overwriting the ngModel directive with your own custom version. This can be very dangerous for people using different versions of Angular, and is not recommended.

Instead, consider refactoring your code to add a decorator to ngModel.

slavafomin commented 8 years ago

Thank you @sscovil! I will keep this in mind during next development cycle.

jrharshath commented 8 years ago

Thanks @sscovil for bringing it up!

@slavafomin When I first started going thru the code to see if I should use this project in my own code, the ngModel overwriting did freak me out a bit. But then I went thru the entire thing and felt more comfortable.

I'm not an angular expert, so I wasn't aware of decorators being even an option.

slavafomin commented 7 years ago

Actually, we are not overwriting ngModel but rather attaching to it using it's name. I think it's a valid use case. Also, I'm trying to interfere with Angular internals as little as possible sticking to the ngModel interface. This approach proved to work pretty fine over ~three years.

@sscovil could you please provide details on how you see this approach dangerous and not recommended.

jrharshath commented 7 years ago

Sidenote: I later realized that declaring another directive as ngModel does not overwrite it, but only attaches another directive to that name. All is well. If the OP is okay, we could close out this issue.