Allow NUMBER and DATETIME to take strings, numbers, dates #410
Conversation
|
|
||
| errors = []; | ||
| msg = bundle.getMessage('num-bad-opt'); | ||
| assert.strictEqual(bundle.formatPattern(msg.value, args, errors), '{NUMBER()}'); | ||
| assert.strictEqual(bundle.formatPattern(msg.value, args, errors), '1475107200000'); |
There was a problem hiding this comment.
This value comes from here:
Line 97 in d3022de
The variable reference resolved fine, but the bad option caused toString to go into the error branch. The error is correctly reported, but I'm not sure if just return this.value is the right fallback. We need to return a string here, so FluentNone is not option.
There was a problem hiding this comment.
Perhaps simulating the result of a FluentNone would be better? I.e. return "{???}".
There was a problem hiding this comment.
Or, we could format using some reasonable defaults.
There was a problem hiding this comment.
In 81cbe84 I changed this to return safe defaults.
There was a problem hiding this comment.
I have two nits, and one issue.
Reading through https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseFloat, I despise parseFloat. I know you don't introduce it into the number scheme. But at least for FluentDateTime, I think we should go for the strict Number() directly.
For FluentNumber, I think we should do the same, but that could be changing behavior, so I'm not sure if that's OK to drive-by. Happy to spin that into a separate issue.
I like the fallback values that you ended up with.
fluent/src/resolver.js
Outdated
| @@ -134,10 +134,10 @@ function VariableReference(scope, {name}) { | |||
| case "string": | |||
| return arg; | |||
| case "number": | |||
| return new FluentNumber(arg); | |||
| return new FluentNumber(parseFloat(arg)); | |||
There was a problem hiding this comment.
This one's not needed, we know we have a number.
fluent/src/types.js
Outdated
| @@ -107,12 +106,14 @@ export class FluentDateTime extends FluentType { | |||
| /** | |||
| * Create an instance of `FluentDateTime` with options to the | |||
| * `Intl.DateTimeFormat` constructor. | |||
| * @param {(Date|number|string)} value | |||
| * @param {number} value | |||
There was a problem hiding this comment.
Make this a bit more precise? timestamp in milliseconds or so?
fluent/src/builtins.js
Outdated
| @@ -44,9 +45,10 @@ function DATETIME([arg], opts) { | |||
| return new FluentNone(`DATETIME(${arg.valueOf()})`); | |||
| } | |||
|
|
|||
| if (arg instanceof FluentDateTime) { | |||
| return new FluentDateTime(arg.valueOf(), merge(arg.opts, opts)); | |||
| let value = parseFloat(arg.valueOf()); | |||
This PR relaxes the type guards inthe
NUMBERandDATETIMEbuiltins. They now accept strings,FluentNumbers, andFluentDateTimes. However, if the native value of the argument results in aNaNduringparseFloat, an error is thrown.Furthermore,
FluentDateTimenow stores the date as a number, which works well with theisNaNcheck mentioned above.DateTimeFormat.format()converts itsDate-typed argument to a number anyways.Found in https://bugzilla.mozilla.org/show_bug.cgi?id=1568914.