tc39 / tc39/proposal-amount

May review

Open
#108 14 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
HTML
Stars
58
Forks
12
Avg merge
2h 35m
Merged PRs (30d)
2

Description

Here's my review so far. I like the direction and approach. A number of problems remain with rounding which will make users believe that Amount can't be used to get reliably rounded results. Also, parts of the spec contradict itself and the explainer, and I'm not sure what the intent there is.

  • My main observation from last meeting still stands: whatever behavior we standardize in the first released version should be the behavior we're happy with long-term, including for things like converting 2m to person-height in the US or France. We shouldn't make users set a flag in the future to get the good result rather than the one we standardized first. Releasing this simultaneously with Intl-Sequence-Units is one possible solution.
  • The explainer and spec contradict each other in the convertTo API. The explainer has unit and usage options, the spec does not.
  • The spec's note in the Amount constructor is incorrect: The spec always stores Number and BigInt arguments directly in [[AmountValue]], regardless of the rounding options.
  • Exponents of 0 are not preserved when handing a string such as "0e-4". I'm happy with that behavior (and wouldn't want to change it) but wanted to make sure that's clear to everyone.
  • Rounding continues to produce surprises:
    • Converting 5 grams to tonnes results in 0.0000049999999999999996 tonnes instead of 0.000005 tonnes. This is caused by double rounding when doing the conversion. There are techniques to do the conversion with just a single rounding at the end which would produce the exactly rounded result as long as the conversion factors aren't too large (i.e. we're not converting solar-masses to daltons).
    • Converting "0.15" meters to meters with conversion options fractionDigits=1 and the default rounding mode "halfEven" produces "1e-1" meters, which is incorrect. The correct answer is "2e-1" meters. Again, multiple rounding is the culprit here.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by comparing the explainer and spec sections for the convertTo API and the Amount constructor, then review the two rounding examples in the issue. Trace how conversion options and conversion factors are specified, and identify the intended behavior for the contradictory cases. Done means the spec and explainer agree and the rounding behavior is unambiguous.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript
Domain
api
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.