Update @woo…/number code docs, define & export type declarations.

[tomalec]

As per the Dependency Extraction Webpack Plugin installing this package in another plugin serves only type checking purposes (see WordPress/gutenberg#35630 (comment)), it would be beneficial to actually define and export those types.

I also updated the docs for the function I find the most confusing and a few return types which I find wrong.

This PR

• Defines NumberConfig data type, export type declarations. (8cdc828)
• Updates formatValue docs, (5d7d53f)
  • explains "based on type"
    Now it clearly explains what and in what type is returned for specific type parameter values.
  • fixes the returned value's type.
    It's not only null or string but may also be a number
• Fix returned type of numberFormat. (baa0c5d)
  locutus/php/strings/number_format seems to always return a String.

Accessibility

n/a - its only code documentation change

Screenshots

Detailed test instructions:

• npm install @woocommerce/number
• Somewhere in your codebase's JSDoc use {import('@woocommerce/number').NumberConfig}

Additional notes:

1. This data type could be used further in @woocommerce/currency to make the code docs shorter.
2. Personally, I find the formatValue API confusing, as it returns type string for type='number' parameter, and type number for type='average'. I'm not sure if that was desired, given the lack of tests. Alternatively, we could change the implementation and make sure it always returns a ?string as the docs stated before. But that may be an effectively breaking change.
3. Also We could set a default value for formatValue's type parameter, to make sure it would never return undefined, which currently may be a result of an invalid param given.

---

No changelog necessary - added entry in package's one.

[tomalec]

See Additional note 2.

[tomalec]

Technically, it may also be undefined if an incorrect type is given. See Additional note 3.

[tomalec]

@psealock Would you mind taking a look?

[psealock]

Personally, I find the formatValue API confusing, as it returns type string for type='number' parameter, and type number for type='average'. I'm not sure if that was desired, given the lack of tests. Alternatively, we could change the implementation and make sure it always returns a ?string as the docs stated before. But that may be an effectively breaking change.

Yeah, that is absolutely confusing. I see what happened there though. number will return something like 4,555,333.00 while average will return an actual number without regard to precision or formatting based on locale. Meanwhile, the function formatValue implies that returned values will be formatted.

The type average is used in wc-admin in Summary Numbers like this

woocommerce-admin/client/analytics/report/orders/config.js

Line 46 in 0cec48d

type: 'average',

and ultimately fed into a <Text /> element. I'd say the breaking change you suggested is the best solution.

woocommerce-admin/packages/components/src/summary/number.js

Lines 114 to 118 in 0cec48d

	<Text variant="title.small" size="20" lineHeight="28px">
	{ ! isNil( value )
	? value
	: __( 'N/A', 'woocommerce-admin' ) }
	</Text>

[psealock]

Also We could set a default value for formatValue's type parameter, to make sure it would never return undefined, which currently may be a result of an invalid param given.

That would require a default numberConfig if the default was number, which sounds good to me.

[adrianduffell]

@tomalec Just checking if this is ready for review of if you are still working on it? The breaking change might be better to implement in a follow-up PR.

We're planning to merge Woo Admin to Core soon and ideally it would be best have this PR completed before then.

[tomalec]

Hi sorry, for the late response, I was away from the keyboard for an extended time :|

On my side, it is ready for review. I'm happy to merge it as is and make separate PRs for potential improvements discussed above.

[tomalec]

Closing in favor of woocommerce/woocommerce#32325