Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Directive uses ngModel incorrectly. #9

Open
jeme opened this issue May 16, 2018 · 0 comments
Open

Directive uses ngModel incorrectly. #9

jeme opened this issue May 16, 2018 · 0 comments

Comments

@jeme
Copy link

jeme commented May 16, 2018

Just wanted to point out that this directive is using ngModel somewhat incorrectly,
This means that the $formatters / $parsers pipelines are broken with the directive (it is far from the only directive I have seen that does this btw, so it is a common misunderstanding)

Generally the case can be demonstrated with: https://plnkr.co/edit/0oFTarMU7HEFYnNC5DFf?p=preview
(Note it's not this directive in action, it's just a demonstration of the differences between using require: ngModel vs scope: { ngModel })

$parsers will never be invoked with they way it's implemented here as the ngModel controller that gets bound with this directive just dangles outside of the scope.

We had to fix this in order for the model to be properly updated before the change event is fired.
(no PR for now, just an inline fix, read bellow why)

As a side effect, you won't have to implement a custom ngChange event as that is all taken care off... The drawback here though is that we won't get the previous value with the ng-change event, depending on the scenario there is a workaround, otherwise it's a trivial thing to fix with another custom directive or inline it as a different event.

Because of the minor drawback, I didn't wan't to provide a PR for it just yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant