-
Notifications
You must be signed in to change notification settings - Fork 27.4k
feat($interpolate): MessageFormat extensions #11152
Conversation
d9c80a2
to
453486b
Compare
@chirayuk thank you for implementing this! Where can I start helping out for the docs? |
250781a
to
d91e802
Compare
Changes since the last time:
|
|
||
var noop = angular['noop'], | ||
isFunction = angular['isFunction'], | ||
toJson = angular['toJson']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use lookup (angular['noop']
) rather than property (angular.noop
) syntax here. Is it because of the Google Closure compilation optimizations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I'm using the ADVANCED_OPTIMIZATIONS
mode of the Closure compiler which will rewrite angular.noop
into something like angular.x
. This module has limited interaction with outside code (and injection has existing array syntax to guard against minification) so there aren't too many places that do this kind of explicit lookup.
There are a bunch of jshint and jscs errors. Can you ensure that |
I think that the filenames should be |
return cachedFn; | ||
} | ||
function parsedFn(context) { return text; }; | ||
var unwatch; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this unwatch
variable declared outside the watchDelegate
function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed. It was cruft from a refactor.
I think it would make it easier to grok this module if it was broken up into a few smaller src files. For instance you have message selector stuff, parsing rules, decorating $interpolate. I can see that the code is generally structured as classes. In the theme of writing stuff that will run the same code base in V2 as V1, perhaps this code could be written as ES6 classes and then transpiled down for this V1 |
case "(": return ")"; | ||
default: return null; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this (and getBeginOperator
) benefit from being written as:
var END_OPERATORS { '{': '}', '[': ']', '(': ')' };
function getEndOperator(opBegin) {
return END_OPERATORS[opBegin];
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A jsperf I wrote a little while back strongly favors the switch statement. (https://jsperf.com/map-vs-switch-vs-if) I don't particularly think one reads any clearer than the other so I went with the switch statement.
A few nitpicks and questions but generally looking good. It would be nice to see a state diagram of the parser somewhere for good measure. |
62c0849
to
2aaaaa6
Compare
|
@petebacondarwin @IgorMinar ready for another review. |
9c2d3b3
to
3507e6f
Compare
@description | ||
|
||
You have repeated a match selection for your plural or select MessageFormat | ||
extension in your interpolation interpolation expression. The different |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interpolation interpolation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Thanks!
Nice one @chirayuk - this looks even better! The only serious concern I have is with the Other than that it looks good to merge, once the few docs fixes are in place. |
Other than the |
Extend interpolation with MessageFormat like syntax. Ref: <https://docs.google.com/a/google.com/document/d/1pbtW2yvtmFBikfRrJd8VAsabiFkKezmYZ_PbgdjQOVU/edit> Example: ```html {{recipients.length, plural, offset:1 =0 {You gave no gifts} =1 { {{ recipients[0].gender, select, male {You gave him a gift.} female {You gave her a gift.} other {You gave them a gift.} }} } one { {{ recipients[0].gender, select, male {You gave him and one other person a gift.} female {You gave her and one other person a gift.} other {You gave them and one other person a gift.} }} } other {You gave {{recipients[0].gender}} and # other people gifts. } }} ``` This is a SEPARATE module so you MUST include angular-messageformat.min.js. In addition, your application module should depend on the "ngMessageFormat". (e.g. angular.module('myApp', ['ngMessageFormat']);) $interpolate automatically gets the new behavior. Quick note on syntax differences from MessageFormat: - MessageFormat directives are always inside {{ }} instead of single { }. This ensures a consistent interpolation syntax (else you could interpolate in more than one way and have to pick one based on feature availability for that syntax.) - The first word inside such syntax can be an arbitrary Angular expression instead of a single identifier. - You can nest them as deep as you want. As mentioned earlier, you would use {{ }} to start the nested interpolation that may optionally include select/plural extensions. - Only "select" and "plural" keywords are currently recognized. - Quoting support is coming in a future commit. - Positional arguments/placeholders are not supported. They don't make sense in Angular templates anyway (they are only helpful when using API calls from a programming language.) - Redefining of the startSymbol and endSymbol used for interpolation is not currently supported yet.
For more detailed information refer to this document: https://docs.google.com/a/google.com/document/d/1pbtW2yvtmFBikfRrJd8VAsabiFkKezmYZ_PbgdjQOVU/edit **Example:** ```html {{recipients.length, plural, offset:1 =0 {You gave no gifts} =1 { {{ recipients[0].gender, select, male {You gave him a gift.} female {You gave her a gift.} other {You gave them a gift.} }} } one { {{ recipients[0].gender, select, male {You gave him and one other person a gift.} female {You gave her and one other person a gift.} other {You gave them and one other person a gift.} }} } other {You gave {{recipients[0].gender}} and # other people gifts. } }} ``` This is a SEPARATE module so you MUST include `angular-messageformat.js` or `angular-messageformat.min.js`. In addition, your application module should depend on the "ngMessageFormat" (e.g. angular.module('myApp', ['ngMessageFormat']);) When you use the `ngMessageFormat`, the $interpolate gets overridden with a new service that adds the new MessageFormat behavior. **Syntax differences from MessageFormat:** - MessageFormat directives are always inside `{{ }}` instead of single `{ }`. This ensures a consistent interpolation syntax (else you could interpolate in more than one way and have to pick one based on the features availability for that syntax.) - The first part of such a syntax can be an arbitrary Angular expression instead of a single identifier. - You can nest them as deep as you want. As mentioned earlier, you would use `{{ }}` to start the nested interpolation that may optionally include select/plural extensions. - Only `select` and `plural` keywords are currently recognized. - Quoting support is coming in a future commit. - Positional arguments/placeholders are not supported. They don't make sense in Angular templates anyway (they are only helpful when using API calls from a programming language.) - Redefining of the startSymbol (`{{`) and endSymbol (`}}`) used for interpolation is not yet supported. Closes angular#11152
See plunker example
Extend interpolation with MessageFormat like syntax.
Ref: https://docs.google.com/a/google.com/document/d/1pbtW2yvtmFBikfRrJd8VAsabiFkKezmYZ_PbgdjQOVU/edit
Example:
This is a SEPARATE module so you MUST include
angular-messageformat.min.js. In addition, your application module
should depend on the "ngMessageFormat".
(e.g. angular.module('myApp', ['ngMessageFormat']);)
$interpolate automatically gets the new behavior.
Quick note on syntax differences from MessageFormat:
single { }. This ensures a consistent interpolation syntax (else you
could interpolate in more than one way and have to pick one based on
feature availability for that syntax.)
expression instead of a single identifier.
would use {{ }} to start the nested interpolation that may optionally
include select/plural extensions.
sense in Angular templates anyway (they are only helpful when using
API calls from a programming language.)
not currently supported yet.