feat(ngMessages): provide support for dynamic message resolution #10676

matsko · 2015-01-07T23:17:11Z

What's the goal here?

To allow multiple message states to use the same message.
To allow structural directives such as ngRepeat, ngIf and ngSwitch to add and remove instances of ngMessage within a ngMessage container.
To allow form controller code to download and dynamically populate message instances using validations from the server.

-- commit message --
Prior to this fix it was impossible to apply a binding to a the
ngMessage directive to represent the name of the error. It was also
not possible to use ngRepeat or any other structural directive to
dynamically update the list of messages. This fix ensures that both
the ngMessage and ngMessages directives automatically update when
any dynamic message data changes.

BREAKING CHANGE:

The ngMessagesInclude attribute is now its own directive and that must
be placed as a child element within the element with the ngMessages
directive. (Keep in mind that the former behaviour of the
ngMessageInclude attribute was that all included ngMessage template
code was placed at the bottom of the element containing the
ngMessages directive; therefore to make this behave in the same way,
place the element containing the ngMessagesInclude directive at the
end of the container containing the ngMessages directive).

<!-- AngularJS 1.3.x -->
<div ng-messages="model.$error" ng-messages-include="remote.html">
  <div ng-message="required">Your message is required</div>
</div>

<!-- AngularJS 1.4.x -->
<div ng-messages="model.$error">
  <div ng-message="required">Your message is required</div>
  <div ng-messages-include="remote.html"></div>
</div>

Closes #10036
Closes #9338

caitp · 2015-01-07T23:18:32Z

This breaks a bunch of stuff...

can we expand on this :)

petebacondarwin · 2015-01-08T13:41:17Z

src/ngMessages/messages.js

@@ -87,12 +95,17 @@
 *          minlength="5"
 *          required />
 *
- *   <div ng-messages="myForm.myEmail.$error" ng-messages-include="my-custom-messages">
+ *   <!-- any ng-message elements that appear BEFORE the ng-message-incldue will


typo: ng-message-incldue

petebacondarwin · 2015-01-08T14:14:48Z

You could add that being able to apply an array of key names to the ngMessage directive is one of the drivers behind the major breaking change that all ngMessage attributes are now expressions.

Without this requirement I believe that we could have just used interpolation for binding since the attribute type would have only been a string - but I might be wrong.

petebacondarwin · 2015-01-08T14:23:19Z

src/ngMessages/messages.js

+       }],
+
+       link: function(scope, element, attrs, ngMessagesCtrl) {
+         ngMessagesCtrl.render = function(collection) {


Why is this method not in the controller already?

petebacondarwin · 2015-01-08T14:35:38Z

Should we move contains() outside of the link function?

matsko · 2015-01-08T15:40:41Z

@petebacondarwin fixed contains.

@petebacondarwin The goal for array-based expressions is noted at the top of the PR.

matsko · 2015-01-08T15:44:37Z

@petebacondarwin all code pushed.

petebacondarwin · 2015-01-08T17:01:20Z

Great @matsko - What I meant about the array expressions was that it would be good to include the motivation for the breaking change in the commit message.

petebacondarwin · 2015-01-12T22:48:22Z

src/ngMessages/messages.js

-            j++;
-          }
-        }
+      require: '^ngMessages',


This can be '^^ngMessages'

Moreover it "should" be since it will then provide an error message if the user puts it in the wrong place.

petebacondarwin · 2015-01-12T23:41:39Z

@matsko - if you can address the comments above this looks good to me. A really fascinating bit of work resulting in a much more Angular-ish way of working: not only the ability to use structural elements but also being able to insert ngMessageInclude directives at will such that the ordering is taken into account. Nice!

Narretz · 2015-01-13T22:46:10Z

src/ngMessages/messages.js

+    *
+    * <!-- or by using element directives -->
+    * <ng-messages for="expression">
+    *   <ng-messages-include src="expressionValue1">...</ng-messages-include>


Shouldn't this be interpolatedValue1 or even {{interpolatedValue}}?

Or, since ng-message now uses expressions, would it make sense for the include to use them, too? ng-includealready uses expressions. Now there's another include directive, but this one uses interpolation.

Good point, but I think for this PR we'll hold off. The main reason is that ngMessageInclude has to keep track of the old message nodes and then destroy them when the watcher fires. I need some time to figure out a good way to make this work. For now it won't be causing a breaking change.

Narretz · 2015-01-13T22:55:38Z

This looks really useful. A few comments:

Is there a specific reason why overwriting ng-message works by putting them before the includes? This seem counterintuitive to me.

Docs:

(introduction paragraph): should mention ngMessageInclude (maybe make all of them links, too)
(Usage): By default, ngMessages will only display one error at a time ... can't hurt to explicitly mention that the first message will be displayed
(Reusing and Overriding Messages): in both script ng-templates , the ng-messageattributes need to be wrapped in single quotes.

petebacondarwin · 2015-01-14T10:21:01Z

@Narretz - the ngMessages directive works by displaying the first matching message. Its a case of first-in wins. So putting messages earlier overwrites later messages. This should be made clearer in the docs as you say.

petebacondarwin · 2015-01-14T10:25:16Z

@matsko and @caitp, regarding not having the breaking change of making ng-message expect an expression now.

Benefits:

Not a breaking change so can be back-ported to 1.3.x
Less verbose syntax for the most frequent use case

Problems:

We would need another attribute for expressions (ng-message-match / ng-message-exp / ng-message-props?)
We would need to find a way to support interpolation in the original ng-message attribute since is not supported right now

matsko · 2015-01-15T07:27:37Z

@petebacondarwin why not just place priority on ng-message-match (or prop or exp) and if that attribute is undefined then we place a watcher on ng-message. This way we can still interpolate the ng-message attr without any performance hit.

Narretz · 2015-02-09T21:24:39Z

Regarding the naming: I have no problem with two attributes, ng-message that takes constant input, and another one that takes an expression.
For the second, I don't like ng-message-match because it's too unspecific. If the difference is no-expression / expression, the new attribute should be called ng-switch-exp(ression).

matsko · 2015-02-11T17:45:05Z

I'm going with ng-message and ng-message-exp.

matsko · 2015-02-11T17:50:34Z

The reasons why we're going with two attributes are because:

{{ curly }} attributes are evaluated at compile. So if a value doesn't exist within the scope at the right time then the message key won't be intercepted. Watching an attribute directly solves this issue.
By having watchers from the start then we can safely stick to that API. If we end up supporting {} object keys then we can support that directly without having to use $parse each time on the interpolated string.

petebacondarwin · 2015-02-11T18:00:02Z

+1 - go for it @matsko

matsko · 2015-02-11T19:18:14Z

Now that we agreed on having two attributes, I think we should also put this feature into 1.3. The only breaking change is ngMessagesInclude code which is a one-line fix.

petebacondarwin · 2015-02-11T19:55:02Z

src/ngMessages/messages.js

@@ -25,8 +33,10 @@
 * <form name="myForm">
 *   <input type="text" ng-model="field" name="myField" required minlength="5" />
 *   <div ng-messages="myForm.myField.$error">
- *     <div ng-message="required">You did not enter a field</div>
- *     <div ng-message="minlength">The value entered is too short</div>
+ *     <div ng-message="require">You did not enter a field</div>


Prior to this fix it was impossible to apply a binding to a the ngMessage directive to represent the name of the error. It was also not possible to use ngRepeat or any other structural directive to dynamically update the list of messages. This feature patch ensures that both ngMessages can render expressions and automatically update when any dynamic message data changes. BREAKING CHANGE: The `ngMessagesInclude` attribute is now its own directive and that must be placed as a **child** element within the element with the ngMessages directive. (Keep in mind that the former behaviour of the ngMessageInclude attribute was that all **included** ngMessage template code was placed at the **bottom** of the element containing the ngMessages directive; therefore to make this behave in the same way, place the element containing the ngMessagesInclude directive at the end of the container containing the ngMessages directive). ```html  <div ng-messages="model.$error" ng-messages-include="remote.html"> <div ng-message="required">Your message is required</div> </div>  <div ng-messages="model.$error"> <div ng-message="required">Your message is required</div> <div ng-messages-include="remote.html"></div> </div> ``` Closes angular#10036 Closes angular#9338

matsko · 2015-02-15T03:40:38Z

Merged as c9a4421

Tobbe · 2015-04-14T09:30:41Z

I noticed that this is marked with milestone 1.3.14, but no traces of this can be found even in 1.3.15. What is holding this back from being introduced in the 1.3 branch?

I cherry-picked this commit on top of the v1.3.x branch and I had no problems applying it, and from my very quick test everything looks to be working as it should.

Tobbe · 2015-04-14T11:43:11Z

(I also needed to apply #11199 as well for my usecase to work, but I had no problem with that one either on my 1.3 branch)

Narretz · 2015-04-14T12:07:51Z

We had / have double milestones for 1.4 beta and 1.3 releases because we try to release simultaneously. However, only the next minor release receives features, the current stable release only receives bug fixes.

Tobbe · 2015-04-14T12:34:14Z

So no chance of this going in to 1.3 then I take it. We'll see if I'll maintain my own fork, or if we'll just have to live without this feature...

In any case, thanks for getting back to me 😃

googlebot added the cla: yes label Jan 7, 2015

matsko force-pushed the ng_messages_refactor branch 4 times, most recently from 3e3634d to b91eb65 Compare January 8, 2015 00:07

petebacondarwin reviewed Jan 8, 2015
View reviewed changes

matsko force-pushed the ng_messages_refactor branch from b91eb65 to 8ece832 Compare January 8, 2015 15:44

Narretz added the component: ngMessages label Jan 12, 2015

petebacondarwin reviewed Jan 12, 2015
View reviewed changes

Narretz reviewed Jan 13, 2015
View reviewed changes

petebacondarwin modified the milestones: 1.4.0-beta.5 / 1.3.14, 1.4.0-beta.4 / 1.3.13 Feb 9, 2015

matsko force-pushed the ng_messages_refactor branch from 8ece832 to 912b03f Compare February 11, 2015 19:15

This was referenced Feb 11, 2015

ngMessages blows up when there are duplicate child ngMessage keys #9338

Closed

Feature Request: Dynamically add ng-message to ng-messages #10036

Closed

petebacondarwin reviewed Feb 11, 2015
View reviewed changes

matsko force-pushed the ng_messages_refactor branch from 912b03f to 5d55976 Compare February 12, 2015 02:52

matsko closed this Feb 15, 2015

This was referenced Feb 26, 2015

bug(ngMessages): ngMessagesInclude crashes inside ngRepeat angular/angular#816

Closed

bug(ngMessages): ngMessagesInclude crashes inside ngRepeat #11196

Closed

question / request for dynamic ng-messages error #9849

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ngMessages): provide support for dynamic message resolution #10676

feat(ngMessages): provide support for dynamic message resolution #10676

matsko commented Jan 7, 2015

caitp commented Jan 7, 2015

petebacondarwin Jan 8, 2015

matsko Jan 8, 2015

petebacondarwin commented Jan 8, 2015

petebacondarwin Jan 8, 2015

matsko Jan 8, 2015

petebacondarwin commented Jan 8, 2015

matsko commented Jan 8, 2015

matsko commented Jan 8, 2015

petebacondarwin commented Jan 8, 2015

petebacondarwin Jan 12, 2015

petebacondarwin Jan 12, 2015

petebacondarwin commented Jan 12, 2015

Narretz Jan 13, 2015

Narretz Jan 13, 2015

matsko Feb 11, 2015

Narretz commented Jan 13, 2015

petebacondarwin commented Jan 14, 2015

petebacondarwin commented Jan 14, 2015

matsko commented Jan 15, 2015

Narretz commented Feb 9, 2015

matsko commented Feb 11, 2015

matsko commented Feb 11, 2015

petebacondarwin commented Feb 11, 2015

matsko commented Feb 11, 2015

petebacondarwin Feb 11, 2015

matsko commented Feb 15, 2015

Tobbe commented Apr 14, 2015

Tobbe commented Apr 14, 2015

Narretz commented Apr 14, 2015

Tobbe commented Apr 14, 2015

feat(ngMessages): provide support for dynamic message resolution #10676

feat(ngMessages): provide support for dynamic message resolution #10676

Conversation

matsko commented Jan 7, 2015

What's the goal here?

caitp commented Jan 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Jan 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Jan 8, 2015

matsko commented Jan 8, 2015

matsko commented Jan 8, 2015

petebacondarwin commented Jan 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Jan 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz commented Jan 13, 2015

petebacondarwin commented Jan 14, 2015

petebacondarwin commented Jan 14, 2015

matsko commented Jan 15, 2015

Narretz commented Feb 9, 2015

matsko commented Feb 11, 2015

matsko commented Feb 11, 2015

petebacondarwin commented Feb 11, 2015

matsko commented Feb 11, 2015

Choose a reason for hiding this comment

matsko commented Feb 15, 2015

Tobbe commented Apr 14, 2015

Tobbe commented Apr 14, 2015

Narretz commented Apr 14, 2015

Tobbe commented Apr 14, 2015