Message: Support ICU MessageFormat using slexaxton/messageformat.js #321

rxaviers · 2014-09-29T21:00:51Z

rxaviers · 2014-09-29T21:24:20Z

The first issue I hit passing Globalize's plural fn to MessageFormat is that MessageFormat's compiled fn can't find variable Globalize. Because, new Function creates a new scope and the compiled code expects the plural function to be globally available.

rxaviers · 2014-09-29T21:29:17Z

Can we change the compiled function to actually expect the plural function as a parameter? I'm hacking/experimenting that now.

SlexAxton · 2014-09-30T04:01:39Z

Hey @rxaviers - looking at this now. Things are looking pretty green. What's your read on what we should change in messageformat to support your use-case?

rxaviers · 2014-10-01T01:57:23Z

@SlexAxton, thanks for asking.

I need to pass a plural function to messageformat. Currently, this is only possible if the function is self-contained or if it refers to globals. A simple test case demonstrating what I mean: https://gist.github.com/rxaviers/ec8443f489f1d8d0d454.

So, the very first minimal change I need is to be able to pass the plural function as a parameter to the compiled function. That's the easiest part and I should be able to create a custom new Function myself using mf.precompile(mf.parse()). Although, both the parse and runtime functions refer a global variable.

In this PR, I add a "runtime" hack to change that by replacing such occurrences, and it works as expected.

I've initiated a change in this branch. But, some tests still fail (I'm running tests with make publish && make).

Overall, is this something that could be changed in messageformat in your opinion?

Ideally, I wondered if I could access the core MessageFormat parts more directly. If your code were modularized like modernizr or this repo, I could access the source pieces instead of the bundle. Perhaps that would make it easier. Since, MessageFormat currently involves a build step, nothing would change on that regard. What do you think?

rxaviers · 2014-10-01T14:21:28Z

@SlexAxton If you are open to changing your API slightly (approximating to Strawman Draft), we could have messageformat/messageformat#65. This would definitely solve the above.

eemeli · 2014-10-01T20:21:27Z

@rxaviers, I really like what you did replacing i18n with local functions. I hadn't thought of that when I put together the current formulation.

So I sort of completed what you started, here. That includes updates to bin/messageformat.js and the tests, so it compiles and tests out all right. I took the liberty to refactor your var runtimeFunctions and mf.renderRuntime to an object mf.runtime with a toString() method (where mf = new MessageFormat(...)), as I thought it made more semantic sense that way. Also, var g is now f.lc, and includes all the defined locales -- they're needed to make the locale choice a runtime rather than compile time option, so bin/messageformat.js can produce output supporting multiple simultaneous locales.

Internally, mf.runtime.toString() currently refers to MessageFormat.locale to make sure that MessageFormat.locale = ... won't create problems if run after declaring mf, but I'm not sure if that isn't too much handholding, and hobbling the possibility to define a custom plural function via mf.runtime.lc. What do you think?

Ideally, I wondered if I could access the core MessageFormat parts more directly. If your code were modularized like modernizr or this repo, I could access the source pieces instead of the bundle.

I know the question wasn't addressed to me, but can you clarify what you mean? Because I don't think there's really that much that can be modularized further from messageformat.js...

SlexAxton · 2014-10-01T20:39:51Z

Yea, I'm definitely open to supporting those use cases (especially not relying on local or global variables only).

I'm a big fan of this change: https://github.com/eemeli/messageformat.js/compare/fix-x-plural-fn?expand=1 -- When we were initially discussing how to do this, this was what I was trying to advocate for (having the functions just available on the runtime instead of compiled into the output), so I'm very much in favor of these changes.

Ideally, I wondered if I could access the core MessageFormat parts more directly. If your code were modularized like modernizr or this repo, I could access the source pieces instead of the bundle.

I agree both that modularization is a good thing, but also with @eemeli in that I'm not sure what we could pull out.

rxaviers · 2014-10-01T21:48:49Z

Awesome, excellent guys. Thank you for the prompt answers. @eemeli thanks for the super speedy commits. By the way, mf.runtime and mf.runtime.toString() have been nice changes.

I've updated this branch using the latest code and I could remove all the hacks! Great!

Internally, mf.runtime.toString() currently refers to MessageFormat.locale to make sure that MessageFormat.locale = ... won't create problems if run after declaring mf, but I'm not sure if that isn't too much handholding, and hobbling the possibility to define a custom plural function via mf.runtime.lc. What do you think?

It works for me :). We could rethink that on messageformat/messageformat#65.

@SlexAxton and @eemeli,

Ideally, I wondered if I could access the core MessageFormat parts more directly. If your code were modularized like modernizr or this repo, I could access the source pieces instead of the bundle.

I don't want to distract you with that. So, we can first focus on messageformat/messageformat#65. So, please, ignore it for now.

jzaefferer · 2014-12-11T17:11:14Z

@SlexAxton any update on the license? That's the only blocker for this to land (adding support for message format to Globalize).

SlexAxton · 2014-12-11T17:57:37Z

If someone can PR adding the necessary license to the messageformat repo,
I'll merge it right away.

On Thu, Dec 11, 2014, 9:11 AM Jörn Zaefferer [email protected]
wrote:

@SlexAxton https://github.com/SlexAxton any update on the license?
That's the only blocker for this to land (adding support for message format
to Globalize).

—
Reply to this email directly or view it on GitHub
#321 (comment).

rxaviers · 2014-12-12T18:02:29Z

@SlexAxton messageformat/messageformat#83

jzaefferer · 2014-12-18T12:07:53Z

bower.json

@@ -15,6 +15,7 @@
  "devDependencies": {
    "make-plural": "eemeli/make-plural.js#2.1.2",
    "es5-shim": "3.4.0",
+    "messageformat": "SlexAxton/messageformat.js#302a46a38d511de3b499f9ce0d18289e9fbd92b3",


This will be replaced with a tag once the liencese change is tagged, right?

Correct! Or at least to point to debeaf4 (new license in place)

jzaefferer · 2014-12-18T12:34:41Z

examples/amd-bower/.bowerrc

@@ -1,6 +1,6 @@
 {
  "scripts": {
    "preinstall": "npm install cldr-data-downloader",
-    "postinstall": "./node_modules/cldr-data-downloader/bin/download.js -i bower_components/cldr-data/index.json -o bower_components/cldr-data/"
+    "postinstall": "node ./node_modules/cldr-data-downloader/bin/download.js -i bower_components/cldr-data/index.json -o bower_components/cldr-data/"


Doesn't really belong in this PR, does it?

Ops it doesn't. I've extracted this change and landed this piece on master. Rebased.

Why is it still showing the diff here?

I've updated origin/master. But, the it still show up to me also. Although:

It doesn't show up to me running git diff origin/master..origin/fix-265-message-format. So, a github delay/issue?

jzaefferer · 2014-12-18T12:54:48Z

test/functional/message/message-formatter.js

+
+QUnit.test( "should return the loaded translation", function( assert ) {
+	assert.messageFormatter( "pt", "amen", "Amém" );
+	assert.messageFormatter( "zh", "amen", "阿门" );


Could add an assertion that uses the root locale.

That is, merge this test with the one below, since they use the same test data/setup.

They are in two different sections, because they test two different levels of implementation support.

rxaviers · 2014-12-19T15:49:01Z

All done.

…at.js

rxaviers mentioned this pull request Sep 30, 2014

Support ICU MessageFormat #265

Closed

This was referenced Oct 1, 2014

Add support for selectordinal statements messageformat/messageformat#66

Merged

Finally get utility functions into local scope messageformat/messageformat#67

Merged

rxaviers force-pushed the fix-265-message-format branch 2 times, most recently from 8afd6bd to 18a5d50 Compare October 8, 2014 21:07

scottgonzalez mentioned this pull request Dec 12, 2014

Change license from Apache v2.0 to MIT messageformat/messageformat#83

Merged

rxaviers force-pushed the fix-265-message-format branch 2 times, most recently from cce9b7a to 56edc4f Compare December 17, 2014 10:22

jzaefferer reviewed Dec 18, 2014
View reviewed changes

rxaviers force-pushed the fix-265-message-format branch from f182ef5 to 309ea0d Compare December 18, 2014 12:54

jzaefferer reviewed Dec 18, 2014
View reviewed changes

Message: Support ICU MessageFormat using slexaxton/messageformat.js

6319478

rxaviers force-pushed the fix-265-message-format branch from 0a22be1 to 6319478 Compare December 19, 2014 15:50

rxaviers added 2 commits December 19, 2014 14:12

fixup! Message: Support ICU MessageFormat using slexaxton/messageform…

5308338

…at.js

fixup! Message: Support ICU MessageFormat using slexaxton/messageform…

b14d3b9

…at.js

rxaviers closed this in 6968078 Dec 19, 2014

rxaviers deleted the fix-265-message-format branch January 12, 2015 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Message: Support ICU MessageFormat using slexaxton/messageformat.js #321

Message: Support ICU MessageFormat using slexaxton/messageformat.js #321

rxaviers commented Sep 29, 2014

rxaviers commented Sep 29, 2014

rxaviers commented Sep 29, 2014

SlexAxton commented Sep 30, 2014

rxaviers commented Oct 1, 2014

rxaviers commented Oct 1, 2014

eemeli commented Oct 1, 2014

SlexAxton commented Oct 1, 2014

rxaviers commented Oct 1, 2014

jzaefferer commented Dec 11, 2014

SlexAxton commented Dec 11, 2014

rxaviers commented Dec 12, 2014

jzaefferer Dec 18, 2014

rxaviers Dec 18, 2014

jzaefferer Dec 18, 2014

rxaviers Dec 18, 2014

jzaefferer Dec 18, 2014

rxaviers Dec 19, 2014

jzaefferer Dec 18, 2014

jzaefferer Dec 18, 2014

rxaviers Dec 18, 2014

rxaviers commented Dec 19, 2014

Message: Support ICU MessageFormat using slexaxton/messageformat.js #321

Message: Support ICU MessageFormat using slexaxton/messageformat.js #321

Conversation

rxaviers commented Sep 29, 2014

rxaviers commented Sep 29, 2014

rxaviers commented Sep 29, 2014

SlexAxton commented Sep 30, 2014

rxaviers commented Oct 1, 2014

rxaviers commented Oct 1, 2014

eemeli commented Oct 1, 2014

SlexAxton commented Oct 1, 2014

rxaviers commented Oct 1, 2014

jzaefferer commented Dec 11, 2014

SlexAxton commented Dec 11, 2014

rxaviers commented Dec 12, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rxaviers commented Dec 19, 2014