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

feat($interpolate): MessageFormat extensions #11152

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@chirayuk
Contributor

chirayuk commented Feb 23, 2015

See plunker example

Extend interpolation with MessageFormat like syntax.

Ref: https://docs.google.com/a/google.com/document/d/1pbtW2yvtmFBikfRrJd8VAsabiFkKezmYZ_PbgdjQOVU/edit

Example:

{{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.

@googlebot googlebot added the cla: yes label Feb 23, 2015

@PascalPrecht

This comment has been minimized.

Show comment
Hide comment
@PascalPrecht

PascalPrecht Feb 23, 2015

Contributor

@chirayuk thank you for implementing this! Where can I start helping out for the docs?

Contributor

PascalPrecht commented Feb 23, 2015

@chirayuk thank you for implementing this! Where can I start helping out for the docs?

@chirayuk

This comment has been minimized.

Show comment
Hide comment
@chirayuk

chirayuk Mar 5, 2015

Contributor

Changes since the last time:

  • Separate self contained module. Use angular-messageformat.js and depend on ngMessageFormat.
  • NO changes to existing $interpolate or $parse. I did a major refactor and the core no longer knows or cares about this. However, this is a private service and relies on core implementation.
  • Full $interpolate replacement
  • Supports mustHaveExpression, trustedContext and allOrNothing.
  • Escaping compatible with $interpolate.
  • Includes the entire contents of interpolateSpec.js to test the replacement behavior. See https://github.com/angular/angular.js/pull/11152/files#diff-3746da2fc13376182459459ead442464R229
  • Support for # symbol to mean (number-offset) inside plural messages. (For compatibility with messageformat.)
  • Escaping support inside MessageFormat expressions.
  • Accurate recognition of angular expressions—understands subexpressions, strings/string escapes, etc.
  • Compilation with Closure's ADVANCED_OPTIMIZATIONS mode.
  • And other stuff I can't remember. :)
Contributor

chirayuk commented Mar 5, 2015

Changes since the last time:

  • Separate self contained module. Use angular-messageformat.js and depend on ngMessageFormat.
  • NO changes to existing $interpolate or $parse. I did a major refactor and the core no longer knows or cares about this. However, this is a private service and relies on core implementation.
  • Full $interpolate replacement
  • Supports mustHaveExpression, trustedContext and allOrNothing.
  • Escaping compatible with $interpolate.
  • Includes the entire contents of interpolateSpec.js to test the replacement behavior. See https://github.com/angular/angular.js/pull/11152/files#diff-3746da2fc13376182459459ead442464R229
  • Support for # symbol to mean (number-offset) inside plural messages. (For compatibility with messageformat.)
  • Escaping support inside MessageFormat expressions.
  • Accurate recognition of angular expressions—understands subexpressions, strings/string escapes, etc.
  • Compilation with Closure's ADVANCED_OPTIMIZATIONS mode.
  • And other stuff I can't remember. :)
Show outdated Hide outdated src/ngMessageFormat/messageformat.js
var noop = angular['noop'],
isFunction = angular['isFunction'],
toJson = angular['toJson'];

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

Why use lookup (angular['noop']) rather than property (angular.noop) syntax here. Is it because of the Google Closure compilation optimizations?

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

Why use lookup (angular['noop']) rather than property (angular.noop) syntax here. Is it because of the Google Closure compilation optimizations?

This comment has been minimized.

@chirayuk

chirayuk Mar 11, 2015

Contributor

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.

@chirayuk

chirayuk Mar 11, 2015

Contributor

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.

Show outdated Hide outdated src/ngMessageFormat/messageformat.js
toJson = angular['toJson'];
function stringify(value) {
if (value == null) { return ''; }

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

== is to catch undefined too?

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

== is to catch undefined too?

This comment has been minimized.

@chirayuk

chirayuk Mar 11, 2015

Contributor

Yup. Added a comment to make it clear.

@chirayuk

chirayuk Mar 11, 2015

Contributor

Yup. Added a comment to make it clear.

Show outdated Hide outdated src/ngMessageFormat/messageformat.js
}
var PARSE_CACHE_FOR_TEXT_LITERALS = new Object(null);
var PARSE_CACHE_FOR_INTERPOLATIONS = new Object(null);

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

Do you mean to use Object.create(null) here, which creates objects with no prototype, rather than new Object(null), which is really just an empty object: {}?

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

Do you mean to use Object.create(null) here, which creates objects with no prototype, rather than new Object(null), which is really just an empty object: {}?

This comment has been minimized.

@chirayuk

chirayuk Mar 11, 2015

Contributor

Fixed. Thanks for catching that!

@chirayuk

chirayuk Mar 11, 2015

Contributor

Fixed. Thanks for catching that!

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Mar 5, 2015

Member

There are a bunch of jshint and jscs errors. Can you ensure that grunt ci-checks passes?

Member

petebacondarwin commented Mar 5, 2015

There are a bunch of jshint and jscs errors. Can you ensure that grunt ci-checks passes?

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Mar 5, 2015

Member

I think that the filenames should be messageFormat.js and messageFormatSpec.js for consistency with other angular files.

Member

petebacondarwin commented Mar 5, 2015

I think that the filenames should be messageFormat.js and messageFormatSpec.js for consistency with other angular files.

Show outdated Hide outdated src/ngMessageFormat/messageformat.js
return cachedFn;
}
function parsedFn(context) { return text; };
var unwatch;

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

Why is this unwatch variable declared outside the watchDelegate function?

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

Why is this unwatch variable declared outside the watchDelegate function?

This comment has been minimized.

@chirayuk

chirayuk Mar 11, 2015

Contributor

Removed. It was cruft from a refactor.

@chirayuk

chirayuk Mar 11, 2015

Contributor

Removed. It was cruft from a refactor.

Show outdated Hide outdated src/ngMessageFormat/messageformat.js
};
PARSE_CACHE_FOR_TEXT_LITERALS[text] = parsedFn;
if (goog.DEBUG) {
// Only needed in order to pretend to be $interpolate for tests copied from interpolateSpec.js

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

These properties are also used by $compile.$$addBindingInfo() to allow Protractor to find elements by binding. We need to have these here at least for the unminified build. Is it true that goog.DEBUG will be truthy except in the minified build or is it only truthy when running the tests?

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

These properties are also used by $compile.$$addBindingInfo() to allow Protractor to find elements by binding. We need to have these here at least for the unminified build. Is it true that goog.DEBUG will be truthy except in the minified build or is it only truthy when running the tests?

This comment has been minimized.

@chirayuk

chirayuk Mar 11, 2015

Contributor

Ah. I knew there was something I was missing but hadn't found any helpful comments to guide me when I had grepped our code.

Yes, goog.DEBUG is false only in the minified version and if true otherwise which should allow Protractor to work correctly. I went ahead and renamed the symbol to goog.MINIFIED and switched the bool value to make this clear. In the minified build, because I'm using ADVANCED_OPTIMIZATIONS, the entire if block is optimized away.

@chirayuk

chirayuk Mar 11, 2015

Contributor

Ah. I knew there was something I was missing but hadn't found any helpful comments to guide me when I had grepped our code.

Yes, goog.DEBUG is false only in the minified version and if true otherwise which should allow Protractor to work correctly. I went ahead and renamed the symbol to goog.MINIFIED and switched the bool value to make this clear. In the minified build, because I'm using ADVANCED_OPTIMIZATIONS, the entire if block is optimized away.

Show outdated Hide outdated src/ngMessageFormat/messageformat.js
}
function subtractOffset(expressionFn, offset) {
if (offset == 0) {

This comment has been minimized.

@petebacondarwin
@petebacondarwin

This comment has been minimized.

@chirayuk

chirayuk Mar 11, 2015

Contributor

Fixed.

@chirayuk

chirayuk Mar 11, 2015

Contributor

Fixed.

Show outdated Hide outdated src/ngMessageFormat/messageformat.js
this.lastMessage = void 0;
this.messageFnWatcher = noop;
var expressionFnListener = function(newValue, oldValue) { return self.expressionFnListener(newValue, oldValue); };
this.expressionFnWatcher = scope['$watch'](msgSelector.expressionFn, expressionFnListener, objectEquality);

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

Could we not simplify some of these uses of instance methods as callbacks using Function.bind()...

this.expressionFnWatcher = scope['$watch'](msgSelector.expressionFn, this.expressionFnListener.bind(this), objectEquality);
@petebacondarwin

petebacondarwin Mar 5, 2015

Member

Could we not simplify some of these uses of instance methods as callbacks using Function.bind()...

this.expressionFnWatcher = scope['$watch'](msgSelector.expressionFn, this.expressionFnListener.bind(this), objectEquality);

This comment has been minimized.

@chirayuk

chirayuk Mar 11, 2015

Contributor

I decided against it as a matter of balancing style and performance. From the perspective of style, I think the entire file should stick to just one approach—either using Function.bind or an explicit wrapper—instead of having to pick at each call site. It turns out that Function.bind is typically about 100x slower than the method used here. I especially do not want to someday realize that such calls were chained in some flow paying 100x times 100x for the call. The current approach also has the advantage that a .toString() on the wrapper function helps with debugging while Function.bind is useless for it. So I've made a style choice to use the simple wrapper functions everywhere in this module. (Ref: http://stackoverflow.com/questions/18895305/will-function-prototype-bind-always-be-slow)

@chirayuk

chirayuk Mar 11, 2015

Contributor

I decided against it as a matter of balancing style and performance. From the perspective of style, I think the entire file should stick to just one approach—either using Function.bind or an explicit wrapper—instead of having to pick at each call site. It turns out that Function.bind is typically about 100x slower than the method used here. I especially do not want to someday realize that such calls were chained in some flow paying 100x times 100x for the call. The current approach also has the advantage that a .toString() on the wrapper function helps with debugging while Function.bind is useless for it. So I've made a style choice to use the simple wrapper functions everywhere in this module. (Ref: http://stackoverflow.com/questions/18895305/will-function-prototype-bind-always-be-slow)

Show outdated Hide outdated src/ngMessageFormat/messageformat.js
};
MessageFormatParser.prototype.rulePluralStyle = function rulePluralStyle() {
this.choices = {};

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

Object.create(null)?

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

Object.create(null)?

This comment has been minimized.

@chirayuk

chirayuk Mar 11, 2015

Contributor

Right! Fixed.

@chirayuk

chirayuk Mar 11, 2015

Contributor

Right! Fixed.

Show outdated Hide outdated src/ngMessageFormat/messageformat.js
throw $$messageFormatMinErr('dupvalue',
'The choice "{0}" is specified more than once. Duplicate key is at line {1}, column {2} in text "{3}"',
this.choiceKey, position.line, position.column, this.text);
}

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

This check for duplicate choiceKey is repeated code - it could be factored out to DRY this up.

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

This check for duplicate choiceKey is repeated code - it could be factored out to DRY this up.

This comment has been minimized.

@chirayuk

chirayuk Mar 11, 2015

Contributor

Ran it through the dryer. It's now nicely DRY! :)

@chirayuk

chirayuk Mar 11, 2015

Contributor

Ran it through the dryer. It's now nicely DRY! :)

Show outdated Hide outdated src/ngMessageFormat/messageformat.js
'interpolate': function cachedInterpolate(text, mustHaveExpression, trustedContext, allOrNothing) {
return cacheCall(interpolate, text, mustHaveExpression, trustedContext, allOrNothing);
},
'parseMustache': function cachedParseMustache(text, mustHaveExpression, trustedContext, allOrNothing) {

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

The parseMustache code paths never seem to be used. Is this for some unimplemented feature? It seems like we could get rid of a bunch of code if we removed this call and the parser.ruleMustache stuff.

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

The parseMustache code paths never seem to be used. Is this for some unimplemented feature? It seems like we could get rid of a bunch of code if we removed this call and the parser.ruleMustache stuff.

This comment has been minimized.

@chirayuk

chirayuk Mar 11, 2015

Contributor

Yes, it was for a bunch of intermediate commits before I would replace $interpolate completely. I decided to replace $interpolate fully sooner than later so it's no longer required.

As a heads up, in a future commit, I plan to $decorate our original $parse to support these extensions to allow things like ng-bind to work well (as wells as Angular 2 as well.) Luckily, the parser is architected such that one can parse a varied set of things just by picking the correct starting state and "enclosing" expressions such push onto the rule stack before dispatching to them so this shouldn't be big change to the parser.

@chirayuk

chirayuk Mar 11, 2015

Contributor

Yes, it was for a bunch of intermediate commits before I would replace $interpolate completely. I decided to replace $interpolate fully sooner than later so it's no longer required.

As a heads up, in a future commit, I plan to $decorate our original $parse to support these extensions to allow things like ng-bind to work well (as wells as Angular 2 as well.) Luckily, the parser is architected such that one can parse a varied set of things just by picking the correct starting state and "enclosing" expressions such push onto the rule stack before dispatching to them so this shouldn't be big change to the parser.

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Mar 5, 2015

Member

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 ngMessageFormat module?

Member

petebacondarwin commented Mar 5, 2015

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 ngMessageFormat module?

Show outdated Hide outdated src/ngMessageFormat/messageformat.js
case "(": return ")";
default: return null;
}
}

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

Would this (and getBeginOperator) benefit from being written as:

var END_OPERATORS { '{': '}', '[': ']', '(': ')' };
function getEndOperator(opBegin) {
 return END_OPERATORS[opBegin];
}
@petebacondarwin

petebacondarwin Mar 5, 2015

Member

Would this (and getBeginOperator) benefit from being written as:

var END_OPERATORS { '{': '}', '[': ']', '(': ')' };
function getEndOperator(opBegin) {
 return END_OPERATORS[opBegin];
}

This comment has been minimized.

@chirayuk

chirayuk Mar 11, 2015

Contributor

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.

@chirayuk

chirayuk Mar 11, 2015

Contributor

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.

Show outdated Hide outdated src/ngMessageFormat/messageformat.js
'Expecting end of interpolation symbol, "{0}", at line {1}, column {2} in text "{3}"',
'}}', position.line, position.column, this.text);
}
if (this.parsedFn == null) {

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

In what situation would the parsedFn already be defined here?
Or alternatively, why would the parsedFn not have been computed here already?

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

In what situation would the parsedFn already be defined here?
Or alternatively, why would the parsedFn not have been computed here already?

This comment has been minimized.

@chirayuk

chirayuk Mar 11, 2015

Contributor

Added comment elaborating on this.

Here it is:
If we parsed a MessageFormat extension, (e.g. select/plural today, maybe more some other day), then the result has to be a string and those rules would have already set this.parsedFn. If there was no MessageFormat extension, then there is no requirement to stringify the result and parsedFn isn't set. We set it here. While we could have set it unconditionally when exiting the Angular expression, I intend for us to not just replace $interpolate, but also to replace $parse in a future version (so ng-bind can work), and in such a case we do not want to unnecessarily stringify something if it's not going to be used in a string context.

@chirayuk

chirayuk Mar 11, 2015

Contributor

Added comment elaborating on this.

Here it is:
If we parsed a MessageFormat extension, (e.g. select/plural today, maybe more some other day), then the result has to be a string and those rules would have already set this.parsedFn. If there was no MessageFormat extension, then there is no requirement to stringify the result and parsedFn isn't set. We set it here. While we could have set it unconditionally when exiting the Angular expression, I intend for us to not just replace $interpolate, but also to replace $parse in a future version (so ng-bind can work), and in such a case we do not want to unnecessarily stringify something if it's not going to be used in a string context.

Show outdated Hide outdated src/ngMessageFormat/messageformat.js
}
};
// Run through our grammar avoiding deeply nested function call chains.

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

Good idea

@petebacondarwin

This comment has been minimized.

@chirayuk

chirayuk Mar 11, 2015

Contributor

Thanks. :)

@chirayuk

chirayuk Mar 11, 2015

Contributor

Thanks. :)

Show outdated Hide outdated src/ngMessageFormat/messageformat.js
};
MessageFormatParser.prototype.consumeRe = function consumeRe(re) {

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 5, 2015

Member

Can this method not just use matchRe?

return !!this.matchRe(re, false);
@petebacondarwin

petebacondarwin Mar 5, 2015

Member

Can this method not just use matchRe?

return !!this.matchRe(re, false);

This comment has been minimized.

@chirayuk

chirayuk Mar 11, 2015

Contributor

Yes, it can and now it does, given that I can't use the sticky flag. I'm leaving the comment in there.

@chirayuk

chirayuk Mar 11, 2015

Contributor

Yes, it can and now it does, given that I can't use the sticky flag. I'm leaving the comment in there.

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Mar 5, 2015

Member

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.

Member

petebacondarwin commented Mar 5, 2015

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.

@petebacondarwin petebacondarwin added this to the 1.4.0-beta.6 / 1.3.15 milestone Mar 11, 2015

@chirayuk

This comment has been minimized.

Show comment
Hide comment
@chirayuk

chirayuk Mar 12, 2015

Contributor
  • ensure that grunt ci-checks passes
  • filenames should be messageFormat.js and messageFormatSpec.js
  • split into multiple files
  • add error docs
  • expand guide/i18n documentation
  • show state diagram
  • This is a good idea. I will do this in a follow up commit.
  • Use ES6 classes and transpile
  • This is out of scope for this commit. I intend to do so in a future commit via some mechanical refactoring changes.
Contributor

chirayuk commented Mar 12, 2015

  • ensure that grunt ci-checks passes
  • filenames should be messageFormat.js and messageFormatSpec.js
  • split into multiple files
  • add error docs
  • expand guide/i18n documentation
  • show state diagram
  • This is a good idea. I will do this in a follow up commit.
  • Use ES6 classes and transpile
  • This is out of scope for this commit. I intend to do so in a future commit via some mechanical refactoring changes.
@chirayuk

This comment has been minimized.

Show comment
Hide comment
@chirayuk

chirayuk Mar 12, 2015

Contributor

@petebacondarwin @IgorMinar ready for another review.

Contributor

chirayuk commented Mar 12, 2015

@petebacondarwin @IgorMinar ready for another review.

Show outdated Hide outdated docs/content/error/$interpolate/dupvalue.ngdoc
@description
You have repeated a match selection for your plural or select MessageFormat
extension in your interpolation interpolation expression. The different

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 12, 2015

Member

interpolation interpolation?

@petebacondarwin

petebacondarwin Mar 12, 2015

Member

interpolation interpolation?

This comment has been minimized.

@chirayuk

chirayuk Mar 16, 2015

Contributor

Fixed. Thanks!

@chirayuk

chirayuk Mar 16, 2015

Contributor

Fixed. Thanks!

Show outdated Hide outdated docs/content/error/$interpolate/logicbug.ngdoc
@fullName Bug in ngMessageFormat module
@description
You've just hit a bug in the ngMessageFormat module provided by provided by

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 12, 2015

Member

provided by provided by?

@petebacondarwin

petebacondarwin Mar 12, 2015

Member

provided by provided by?

This comment has been minimized.

@chirayuk

chirayuk Mar 16, 2015

Contributor

Fixed. Thanks!

@chirayuk

chirayuk Mar 16, 2015

Contributor

Fixed. Thanks!

Show outdated Hide outdated docs/content/error/$interpolate/nochgmustache.ngdoc
`ngMessageFormat` currently does not support redefinition of the
startSymbol/endSymbol used by `$interpolate`. If this is affecting you, please
file an issue and mention @chirayuk on it. This is intended to be fixed in a
future commit and the github issue will help gauage urgency.

This comment has been minimized.

@petebacondarwin
@petebacondarwin

This comment has been minimized.

@chirayuk

chirayuk Mar 16, 2015

Contributor

Fixed. Thanks!

@chirayuk

chirayuk Mar 16, 2015

Contributor

Fixed. Thanks!

Your interpolation expression with a MessageFormat extension for either
"plural" or "select" (typically used for gender selection) does not contain a
message for the choice "other". Using either select or plural MessageFormat
extensions require that you provide a message for the selection "other".

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 12, 2015

Member

So for gender selectors you would need to provide: male, female and other? Or would you just put male and other?

@petebacondarwin

petebacondarwin Mar 12, 2015

Member

So for gender selectors you would need to provide: male, female and other? Or would you just put male and other?

This comment has been minimized.

@chirayuk

chirayuk Mar 16, 2015

Contributor

Yup. You provide all the choices that you expect to match and "other" if there is no match. For plurals, for instance, you can omit categories that you think can't occur, but you must still include "other". For gender, you should include all the choices which might require a different message. Since male, female and other have different messages, typically, would include all three. However, for a message like, "You have no posts", you might include just other as the "You" in English is gender neutral but might not be in a different language.

@chirayuk

chirayuk Mar 16, 2015

Contributor

Yup. You provide all the choices that you expect to match and "other" if there is no match. For plurals, for instance, you can omit categories that you think can't occur, but you must still include "other". For gender, you should include all the choices which might require a different message. Since male, female and other have different messages, typically, would include all three. However, for a message like, "You have no posts", you might include just other as the "You" in English is gender neutral but might not be in a different language.

Show outdated Hide outdated docs/content/guide/i18n.ngdoc
upon (shipped separately as `angular-messageFormat.min.js` and
`angular-messageFormat.js`.) A current limitation of this
module is that it is not compatible with `$interpolate` used
with redefined start and end symbols.

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 12, 2015

Member

I would rephrase this slightly to:

A current limitation of the ngMessageFormat module, is that it does not support redefining the $interpolate start and end symbols. Only the default {{ and }} are allowed.

@petebacondarwin

petebacondarwin Mar 12, 2015

Member

I would rephrase this slightly to:

A current limitation of the ngMessageFormat module, is that it does not support redefining the $interpolate start and end symbols. Only the default {{ and }} are allowed.

This comment has been minimized.

@chirayuk

chirayuk Mar 16, 2015

Contributor

Done.

@chirayuk

chirayuk Mar 16, 2015

Contributor

Done.

Show outdated Hide outdated docs/content/guide/i18n.ngdoc
been designed to be backwards compatible with existing
AngularJS interpolation expressions. The key rule is simply
this: **All interpolations are done inside double curlies.**
Consider valid MessageFormat syntax. Anywhere in that

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 12, 2015

Member

The sentence

Consider valid MessageFomat syntax.

Doesn't seem to fit here?

@petebacondarwin

petebacondarwin Mar 12, 2015

Member

The sentence

Consider valid MessageFomat syntax.

Doesn't seem to fit here?

This comment has been minimized.

@chirayuk

chirayuk Mar 16, 2015

Contributor

Rewrote it to make a little more sense.

@chirayuk

chirayuk Mar 16, 2015

Contributor

Rewrote it to make a little more sense.

Show outdated Hide outdated docs/content/guide/i18n.ngdoc
limited to simple identifiers for substitutions**. Because
you are using double curlies, you can stick in any arbitrary
interpolation syntax there, including nesting more
MessageFormat! Some examples will make this clear. In the

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 12, 2015

Member

including nesting more MessageFormat expressions!

@petebacondarwin

petebacondarwin Mar 12, 2015

Member

including nesting more MessageFormat expressions!

This comment has been minimized.

@chirayuk

chirayuk Mar 16, 2015

Contributor

Done.

@chirayuk

chirayuk Mar 16, 2015

Contributor

Done.

Show outdated Hide outdated docs/content/guide/i18n.ngdoc
```
While I won't be teaching you MessageFormat here, you will
not that the `#` symbol works as expected. You could have

This comment has been minimized.

@petebacondarwin
@petebacondarwin

This comment has been minimized.

@chirayuk

chirayuk Mar 16, 2015

Contributor

Fixed!

@chirayuk

chirayuk Mar 16, 2015

Contributor

Fixed!

This syntax extension, while based on MessageFormat, has
been designed to be backwards compatible with existing
AngularJS interpolation expressions. The key rule is simply
this: **All interpolations are done inside double curlies.**

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 12, 2015

Member

I think it is worth, at some point in this guide, calling out the fact that it is the comma character that triggers the expression to be considered as a messageFormat expression.

@petebacondarwin

petebacondarwin Mar 12, 2015

Member

I think it is worth, at some point in this guide, calling out the fact that it is the comma character that triggers the expression to be considered as a messageFormat expression.

This comment has been minimized.

@chirayuk

chirayuk Mar 16, 2015

Contributor

Good point! Done.

@chirayuk

chirayuk Mar 16, 2015

Contributor

Good point! Done.

$interpolateMinErr.interr = function(text, err) {
return $interpolateMinErr('interr', "Can't interpolate: {0}\n{1}", text, err.toString());
};

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 12, 2015

Member

The downside of DRYing up these errors is that they cause yet more warnings when minifying the files...

>> build/angular-sanitize.js minified into build/angular-sanitize.min.js
build/angular-messageFormat.js:290: WARNING - Throw expression is not a minErr instance.
    throw $interpolateMinErr['noconcat'](originalText);
    ^

0 error(s), 1 warning(s)
>> build/angular-messageFormat.js minified into build/angular-messageFormat.min.js
>> build/angular-animate.js minified into build/angular-animate.min.js
build/angular.js:4297: WARNING - Throw expression is not a minErr instance.
          throw err;
          ^

build/angular.js:9967: WARNING - Throw expression is not a minErr instance.
            throw e;
            ^

build/angular.js:10290: WARNING - Throw expression is not a minErr instance.
          throw $interpolateMinErr.noconcat(text);
          ^

build/angular.js:11488: WARNING - Throw expression is not a minErr instance.
        throw e;
        ^

build/angular.js:15322: WARNING - Throw expression is not a minErr instance.
            throw e;
            ^

0 error(s), 5 warning(s)
@petebacondarwin

petebacondarwin Mar 12, 2015

Member

The downside of DRYing up these errors is that they cause yet more warnings when minifying the files...

>> build/angular-sanitize.js minified into build/angular-sanitize.min.js
build/angular-messageFormat.js:290: WARNING - Throw expression is not a minErr instance.
    throw $interpolateMinErr['noconcat'](originalText);
    ^

0 error(s), 1 warning(s)
>> build/angular-messageFormat.js minified into build/angular-messageFormat.min.js
>> build/angular-animate.js minified into build/angular-animate.min.js
build/angular.js:4297: WARNING - Throw expression is not a minErr instance.
          throw err;
          ^

build/angular.js:9967: WARNING - Throw expression is not a minErr instance.
            throw e;
            ^

build/angular.js:10290: WARNING - Throw expression is not a minErr instance.
          throw $interpolateMinErr.noconcat(text);
          ^

build/angular.js:11488: WARNING - Throw expression is not a minErr instance.
        throw e;
        ^

build/angular.js:15322: WARNING - Throw expression is not a minErr instance.
            throw e;
            ^

0 error(s), 5 warning(s)

This comment has been minimized.

@chirayuk

chirayuk Mar 16, 2015

Contributor

Fixed it. Not super happy with the fix. It adds a stack frame for noconcat and also breaks the symmetry a bit. I think we should really make the minErr pass of the closure compiler smarter but that's a lot more work at this point in time. I think that the current choice is an acceptable compromise for the present time.

@chirayuk

chirayuk Mar 16, 2015

Contributor

Fixed it. Not super happy with the fix. It adds a stack frame for noconcat and also breaks the symmetry a bit. I think we should really make the minErr pass of the closure compiler smarter but that's a lot more work at this point in time. I think that the current choice is an acceptable compromise for the present time.

• Whitespace ignored around syntax except for offset:N.
• Escaping for curlies and the # symbol.
• # symbol value.
• # symbol value when gender is nested inside plural.

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 12, 2015

Member

This seems like quite an important use case. I guess the actual code does already work. We should prioritize this test after we get this merged.

@petebacondarwin

petebacondarwin Mar 12, 2015

Member

This seems like quite an important use case. I guess the actual code does already work. We should prioritize this test after we get this merged.

This comment has been minimized.

@chirayuk

chirayuk Mar 16, 2015

Contributor

Poring over what little information is there on the ICU site does not shed information on whether this should be valid. The 3rd party JS lib supports "#" even in select without a plural. That might be a bug. I don't like the "#" in general because it has a bunch of limitations. So this won't work today and if it break someone, they can file a bug and I'll fix it.

Looking at the actual ICU MessageFormat source test file, test/intltest/tmsgfmt.cpp, I don't see any select message using the # symbol, only plurals and selectordinal (which we don't support.) e.g. line 1863

void TestMessageFormat::TestSelectOrdinal() {
    IcuTestErrorCode errorCode(*this, "TestSelectOrdinal");
    // Test plural & ordinal together,
    // to make sure that we get the correct cached PluralSelector for each.
    MessageFormat m(
        "{0,plural,one{1 file}other{# files}}, "
        "{0,selectordinal,one{#st file}two{#nd file}few{#rd file}other{#th file}}",
        Locale::getEnglish(), errorCode);
    if (errorCode.logDataIfFailureAndReset("Unable to instantiate MessageFormat")) {
        return;
    }

Let's leave this "unspecified" for now until I can dig into the gory details of the actual implementation to see if it's "really really" supposed to work in select messages.

@chirayuk

chirayuk Mar 16, 2015

Contributor

Poring over what little information is there on the ICU site does not shed information on whether this should be valid. The 3rd party JS lib supports "#" even in select without a plural. That might be a bug. I don't like the "#" in general because it has a bunch of limitations. So this won't work today and if it break someone, they can file a bug and I'll fix it.

Looking at the actual ICU MessageFormat source test file, test/intltest/tmsgfmt.cpp, I don't see any select message using the # symbol, only plurals and selectordinal (which we don't support.) e.g. line 1863

void TestMessageFormat::TestSelectOrdinal() {
    IcuTestErrorCode errorCode(*this, "TestSelectOrdinal");
    // Test plural & ordinal together,
    // to make sure that we get the correct cached PluralSelector for each.
    MessageFormat m(
        "{0,plural,one{1 file}other{# files}}, "
        "{0,selectordinal,one{#st file}two{#nd file}few{#rd file}other{#th file}}",
        Locale::getEnglish(), errorCode);
    if (errorCode.logDataIfFailureAndReset("Unable to instantiate MessageFormat")) {
        return;
    }

Let's leave this "unspecified" for now until I can dig into the gory details of the actual implementation to see if it's "really really" supposed to work in select messages.

Show outdated Hide outdated src/ngMessageFormat/messageFormatCommon.js
PARSE_CACHE_FOR_TEXT_LITERALS[text] = parsedFn;
if (!goog.MINIFIED) {
parsedFn.exp = text; // Needed to pretend to be $interpolate for tests copied from interpolateSpec.js
parsedFn.expressions = []; // Unminified builds require this to call $compile.$$addBindingInfo() which allows Protractor to find elements by binding.

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 12, 2015

Member

I am pretty sure that we need this even for the minified build too. People do use Protractor on the minified builds. In fact, this is a good practice in general for Protractor tests since we can then catch certain errors that can be introduced by minification.

So I think we need this information even in minified builds.

Instead, we could check $compile.$$addBindingInfo to find whether we need to generate this information.

@petebacondarwin

petebacondarwin Mar 12, 2015

Member

I am pretty sure that we need this even for the minified build too. People do use Protractor on the minified builds. In fact, this is a good practice in general for Protractor tests since we can then catch certain errors that can be introduced by minification.

So I think we need this information even in minified builds.

Instead, we could check $compile.$$addBindingInfo to find whether we need to generate this information.

This comment has been minimized.

@chirayuk

chirayuk Mar 16, 2015

Contributor

Ok. Point taken. I'm willing to add it. This should have been trivial to do, except that $compile does not expose debugInfoEnabled on $compile (just on the provider.) Which is fixable on $compile. I can also just add it unconditionally like $interpolate used to do. The latter is super easy to do and I would actually just be deleting and simplifying code.

For AngularDart, I think we had it so that we didn't add debug/test stuff unless you ran with debug/test mode enabled (so the testability service wouldn't work without test mode enabled either.) The equivalent here is debugInfoEnabled. So the right thing to do would be to actually use that one. Sounds good?

@chirayuk

chirayuk Mar 16, 2015

Contributor

Ok. Point taken. I'm willing to add it. This should have been trivial to do, except that $compile does not expose debugInfoEnabled on $compile (just on the provider.) Which is fixable on $compile. I can also just add it unconditionally like $interpolate used to do. The latter is super easy to do and I would actually just be deleting and simplifying code.

For AngularDart, I think we had it so that we didn't add debug/test stuff unless you ran with debug/test mode enabled (so the testability service wouldn't work without test mode enabled either.) The equivalent here is debugInfoEnabled. So the right thing to do would be to actually use that one. Sounds good?

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Mar 12, 2015

Member

Nice one @chirayuk - this looks even better!

The only serious concern I have is with the bindingInfo bits.

Other than that it looks good to merge, once the few docs fixes are in place.

Member

petebacondarwin commented Mar 12, 2015

Nice one @chirayuk - this looks even better!

The only serious concern I have is with the bindingInfo bits.

Other than that it looks good to merge, once the few docs fixes are in place.

@chirayuk

This comment has been minimized.

Show comment
Hide comment
@chirayuk

chirayuk Mar 16, 2015

Contributor

Other than the bindingInfo bits—that are easy to fix—the other comments have been addressed. I'll address the bindingInfo bit this after after I get back from the Japanese Consulate. Thanks for all the comments!

Contributor

chirayuk commented Mar 16, 2015

Other than the bindingInfo bits—that are easy to fix—the other comments have been addressed. I'll address the bindingInfo bit this after after I get back from the Japanese Consulate. Thanks for all the comments!

feat($interpolate): MessageFormat extensions
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.

@chirayuk chirayuk closed this in 1e58488 Mar 17, 2015

netman92 added a commit to netman92/angular.js that referenced this pull request Aug 8, 2015

feat($interpolate): extend interpolation with MessageFormat like syntax
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 #11152
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment