Skip to content

Fix FluentDateTime and FluentNumber primitive conversions #641

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

Merged
merged 5 commits into from
Apr 2, 2025

Conversation

julienw
Copy link
Contributor

@julienw julienw commented Apr 2, 2025

This fixes a problem when using a custom function taking a FluentDateTime as a parameter, like we're doing in the profiler code.

We're relying on being able to convert a FluentDateTime value to a number:
https://github.com/firefox-devtools/profiler/blob/f699b44fcf763bdfb6ebf36c32d668e4d296f5d6/src/utils/l10n-ftl-functions.js#L87-L91

export const SHORTDATE: FluentFunction = (args, _named) => {
  const date = args[0];
  const nowTimestamp = Date.now();


  const timeDifference = nowTimestamp - +date;

used in:
https://github.com/firefox-devtools/profiler/blob/9c8fb5561537894aee2fe8a265d2c127e83ed0e4/locales/en-US/app.ftl#L679-L683

# The function SHORTDATE is specific to the profiler. It changes the rendering
# depending on the proximity of the date from the current date.
# Variables:
#   $date (Date) - The date to display in a shorter way
NumberFormat--short-date = { SHORTDATE($date) }

The good thing is that the code was already present as a toNumber function. It's not clear to me why the original code didn't use valueOf, but doing it in this patch fixes the problem for me.

I actuallty tested locally with our profiler code, that it fixes the issue.

Hope this solution works for you :-)

@eemeli eemeli changed the title Override valueOf in FluentDateTime so that it can be converted to a n… Fix FluentDateTime and FluentNumber primitive conversions Apr 2, 2025
@eemeli
Copy link
Member

eemeli commented Apr 2, 2025

As discussed on Matrix, I took over this PR from @julienw as the proper fix here required a bit more finessing. Effectively, as #636 changed the FluentDateTime value and valueOf() result to not always be a number, casting a FluentDateTime to a number now also tries to call toString() (due to primitive conversion rules), which fails because it's expecting to get a non-empty scope argument.

So to make all of this work and not create further breaking changes, the right thing to do is to:

  1. use Symbol.toPrimitive to more explicitly control the conversion, and
  2. allow toString() to be called without an argument.

@eemeli eemeli requested review from mathjazz and flodolo April 2, 2025 10:34
@eemeli eemeli merged commit 0fc166e into projectfluent:main Apr 2, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants