Runtime: Fix unitFormatter with numberFormatter #719
Conversation
src/core-runtime.js
Outdated
| fn.runtimeKey = key; | ||
| } | ||
| return fn; | ||
| }; |
There was a problem hiding this comment.
Oh! For your proposed solution, we need this function that is similar to common/runtime-bind.js. Can you move this into its own file in common/ and update usage to look like this please (i.e., use a function directly instead of Globalize.)?
PS: It will be challenging to find a name to distinguish this from runtime-bind. Feel free to rename the latter to accommodate a good naming for both.
There was a problem hiding this comment.
Thinking more about it...
Instead of stamping the functions in here, we could do that in the compiler itself (during build time).
There was a problem hiding this comment.
I wanted to avoid having to change the compiler, since it's a separate package.
People would get broken builds if they only update globalize, not the compiler.
There was a problem hiding this comment.
The goal is to make the runtime code (this code) leaner, which is optimal. No worries about updating the compiler. I can help feel you if you have any questions.
There was a problem hiding this comment.
Yeah, but how can you force a project using globalize and globalize-compiler to update the compiler when globalize is updated? If only globalize is updated, it will generate a different runtime key during compilation, and the runtime bundle won't find the correct function.
There was a problem hiding this comment.
We cannot force, indeed, but there are ways to bring this information to the projects. One is documentation and release notes. Another is to use semver and make the existing globalize-compiler to require a peer globalize that goes up until 1.2.3; Only new globalize-compiler will support a peer globalize 1.3.0.
There was a problem hiding this comment.
You're right, fortunately, globalize-compiler currently has a peer dependency on globalize <1.3.0, so that'll get people to upgrade the compiler as well.
|
Thanks @nkovacs!! Great catch! I left a comment above. What do you think? Just let me know if you have any questions about it... |
nkovacs
force-pushed
the
704-unitformatter-runtimekey
branch
from
d8f9ece
to
caa45b8
Compare
| return function( args ) { | ||
| return JSON.stringify( args, function( key, value ) { | ||
| if ( typeof value === "function" ) { | ||
| return value.runtimeKey; // if undefined, the value will be filtered out. |
There was a problem hiding this comment.
Not sure if we should do something about functions that don't have a runtimeKey.
|
Pull request for compiler is up: globalizejs/globalize-compiler#31 |
JSON.stringify omits functions, so the generated runtimeKey did not depend on the value of the numberFormatter option, causing different unitFormatters to have an identical runtimeKey. Fixes globalizejs#704
nkovacs
force-pushed
the
704-unitformatter-runtimekey
branch
from
caa45b8
to
c85af7c
Compare
|
I've rebased on master and uncommented the compiler test. Travis fails because of the missing compiler change. |
|
Ok thanks |
|
@nkovacs, other formatters (e.g., date, relative time) also include a number formatter in their runtime properties. We need to understand why this happens with unit formatter, but not with the other ones... |
|
No other formatter accepts a formatter as an option which should affect the runtimeKey. |
|
True! Thanks |
|
Thanks @nkovacs!
Instead of depending on globalize-compiler to include the runtimeKey info, I've update the runtime function to do it. |
JSON.stringify omits functions, so the generated runtimeKey
did not depend on the value of the numberFormatter option,
causing different unitFormatters to have an identical runtimeKey.
Fixes #704