Runtime support (smaller and quicker code for production)

[rxaviers]

Fix for #398

Related created projects:

• https://github.com/jquery-support/globalize-compiler
• https://github.com/rxaviers/globalize-webpack-plugin

TODO:

• Docs
• Tests
  • Check if runtimeBind is being properly set.
  • Check if the compiled code runs on the supported browsers Test the compiled runtime code on the supported browsers/environments #482.

[dpolivy]

Any timeline for when this (cache support) may be incorporated?

[rxaviers]

Any timeline for when this (cache support) may be incorporated?

I've moved the Cache support for its own PR #470.

[rxaviers]

@jquery/globalize, necessary documentation has been included and cache support has been moved into its own PR. So, this PR is (finally) ready for review.

PS: N.B. it still needs tests.

[rxaviers]

Content team (cc @arthurvr @agcolom @kswedberg who has helped reviewing past doc changes), please just let me know if you have thoughts for improvements. Thanks in advance.

[rxaviers]

PS: Save your time by ignoring the file renames. :P

[jzaefferer]

@rxaviers I don't have the bandwidth to review 12k LOC inbetween. We could do that together some time, let me know when you're available.

Maybe you can pull out the examples changes into a separate PR (like you kinda did with the cache)? That would make reviewing this a lot easier...

[kswedberg]

two-step process (hyphenate two-step)

[rxaviers]

GlobalizeCompiler is tight coupled with Globalize. Perhaps having GlobalizeCompiler to live here (Globalize repository) could help by allowing changes to touch both places at the same time. For comparison, facebook/react keeps some packages the same way.

Thinking more about it, I believe both projects (globalize and globalize-compiler) should be kept in their separate repos. The comparison I gave for react packages makes more sense for globalize and globalize-runtime than for globalize and globalize-compiler. For now, their integration will be tested here by (a) runtime bindings unit tests and (b) app examples that uses the compiler (examples/basic-globalize-compiler and the upcoming examples/app-npm-webpack #464 and examples/app-amd-grunt #465); and their (globalize-compiler) by its own unit tests.

EDIT: we can improve this later at any time.

[rxaviers]

@jzaefferer thanks a lot for all your comments. Most of them have resulted in fix commits. For the others, I have added a reply.

If anyone has any further comments, feel free to make them. We should be good to get this landed.

[jzaefferer]

It looks like this avoids a temporary variable for the assertRuntimeBind called. I recommend refactoring that to actually use the temporary variable, since it'll hold the object that is actually being tested. Then drop the runtimeArgsFn argument and just call whatever other assertion with formatter.runtimeArgs. That removes a level of nesting in several tests along with a noop callback argument in many places.

[rxaviers]

Actually, this allows custom tests to be performed by each calling code. For example: https://github.com/jquery/globalize/pull/436/files#diff-9ee4a6f38e08bc680f4668d25dedaff8R30 and https://github.com/jquery/globalize/pull/436/files#diff-3b257e2c8b293a57b0336b413514f034R110. Please, just let me know if I misinterpreted your comment.

[jzaefferer]

I've reviewed the tests, since I hadn't seen those before. Still doesn't fill me with confidence that someone else is going to be able to contribute to this codebase...

I wanted to check the other additional commits, but since everything has the same commit message, its hard to figure out what each change is about. That's a pretty big drawback of using "fixup!" without any other information. I'll dig some more anyway...

[jzaefferer]

Actually it was just the last set of example updates that had generic commit messages, the rest had some detail in the body. That helped!

[jzaefferer]

Still far from a full review, but at least covered a good chunk. Glad to see that the PR overall got a lot simpler (less LOC changed, less files). Seems mostly due to changes in the example.

[rxaviers]

@jzaefferer thanks for all your reviews so far. They have been pretty helpful.

I believe the feature implemented by this PR is a big plus to the library. It's essential and mandatory to allow developers to use Globalize for production in real projects and should land. So, I suggest that we don't postpone this PR any longer. I acknowledge there are polishings to be done. But, we can always work on improvements as next steps.

I am considering to land what we have here, because I believe it's mature enough to go as phase-1. All improvements are welcome as next step. Anyone that discourages this decision, please say so.

Thanks

[jzaefferer]

Actually, this allows a custom tests to be performed by each calling code. For example: https://github.com/jquery/globalize/pull/436/files#diff-9ee4a6f38e08bc680f4668d25dedaff8R30, https://github.com/jquery/globalize/pull/436/files#diff-3b257e2c8b293a57b0336b413514f034R110. Please, just let me know if I misinterpreted your comment.

Its called synchronously with an argument passed by the caller. Thats trivial to do without the callback. From test/functional/number/number-formatter.js:

util.assertRuntimeBind(
    assert,
    Globalize.numberFormatter(),
    "b468386326",
    "Globalize(\"en\").numberFormatter({})",
    function( runtimeArgs ) {
        assert.equal( JSON.stringify( runtimeArgs[ 0 ] ), "[\"\",null,1,0,3,null,null," +
            "null,3,null,\"\",\"#,##0.###\",\"-#,##0.###\",\"-\",\"\",null,\"∞\",\"NaN\",{" +
            "\".\":\".\",\",\":\",\",\"%\":\"%\",\"+\":\"+\",\"-\":\"-\",\"E\":\"E\",\"‰\":" +
            "\"‰\"},null]"
        );
        assert.ok( "generatorString" in runtimeArgs[ 0 ][ 15 ] );
        assert.equal( runtimeArgs[ 0 ][ 15 ].generatorString(), "numberRound()" );
    }
);

turns to

var formatter = Globalize.numberFormatter() 
util.assertRuntimeBind( assert, formatter, "b468386326", "Globalize(\"en\").numberFormatter({})" );
assert.equal( JSON.stringify( formatter.runtimeArgs[ 0 ] ), "[\"\",null,1,0,3,null,null," +
    "null,3,null,\"\",\"#,##0.###\",\"-#,##0.###\",\"-\",\"\",null,\"∞\",\"NaN\",{" +
    "\".\":\".\",\",\":\",\",\"%\":\"%\",\"+\":\"+\",\"-\":\"-\",\"E\":\"E\",\"‰\":" +
    "\"‰\"},null]"
);
assert.ok( "generatorString" in formatter.runtimeArgs[ 0 ][ 15 ] );
assert.equal( formatter.runtimeArgs[ 0 ][ 15 ].generatorString(), "numberRound()" );

And you can drop the noop function arguments and you get simpler stacktraces.

[rxaviers]

👍 let's get this improved in a subsequent PR. Could you send it please?

[rxaviers]

#486

[jzaefferer]

GitHub says this branch has conflicts that much be resolved.