SAP / ui5-webcomponents

UI5 Web Components - the enterprise-flavored sugar on top of native APIs! Build SAP Fiori user interfaces with the technology of your choice.
https://sap.github.io/ui5-webcomponents/
Apache License 2.0
1.5k stars 260 forks source link

wc-localization 1.13.0 cldr assets missing fields #6990

Closed ac1058 closed 1 year ago

ac1058 commented 1 year ago

Bug Description

When creating instance of LocaleData we are observing the following error:

TypeError: Cannot read property 'standard' of undefined

Upon further investigation I found the some fields are now missing in the new generated cldr assets files namely:

Thanks!

Affected Component

No response

Expected Behaviour

Expected behaviour is that for a locale like en-US calling LocaleData.getDecimalPattern() would not have an undefined error.

Looking at diff of dist/generated/assets/cldr/en.json in wc-localization 1.13.0 and 1.10. I can see that the decimalFormat does not exist in a wc-localization 1.13.0 but decimalFormat does exist in the same en.json file in wc-localization 1.10.3.

I observe in LocaleData.getDecimalPattern() there is no decimalFormat property when using a 1.13.0 release of wc-localization, but it exists in the 1.10.3 version that I previously was consuming.

Isolated Example

No response

Steps to Reproduce

1. 2. 3. ...

Log Output, Stack Trace or Screenshots

Truncated stack trace:

TypeError: Cannot read property 'standard' of undefined

at constructor.getDecimalPattern (node_modules/@ui5/webcomponents-localization/dist/sap/ui/core/LocaleData.js:543:16)
at Function.Object.<anonymous>.NumberFormat.getLocaleFormatOptions (packages/ui5/webcomponents/src/format/NumberFormat.ts:242:42)

Priority

High

UI5 Web Components Version

1.13.0

Browser

Chrome

Operating System

WINDOWS 10

Additional Context

No response

Declaration

scameron commented 1 year ago

This is related to https://github.com/SAP/ui5-webcomponents/pull/6703, which was done to reduce the size of the install footrprint of ui5-wc (it saves something like 4MB in our webpack build).

This change removed any locale information not used internally by ui5-wc, but I guess it does not take into consideration the case where a client application is using ui5-wc localization utilities directly for its own purposes. I didn't actually realize SAC was doing this, but it looks like we're using LocaleData for number formatting.

If this is a supported usage of ui5-wc localization utilities, then I guess it means all of that localization info will need to be put back and we'll need to take the hit of a bigger install. That's too bad, but I guess it's not avoidable.

It looks like we are only using the number formatting and nothing to do with dates. But if this usage is a publicly supported part of ui5-wc, then I guess it's hard to argue that we should expose some things but not others. Not sure...

CC: @vladitasev

vladitasev commented 1 year ago

Hi @ac1058 @scameron ,

We were quite surprised to find out you were using the @ui5/webcomponents-localization classes directly.

Let me give you some background on the whole story: the @ui5/webcomponents-localization package copies some date/time-related classes from OpenUI5, transpiles them to ES6, and overrides some core OpenUI5 functionality (namely LoaderExtensions to make the OpenUI5 classes use our CLDR assets). Until now, these classes were only used internally by the components, and we didn't suspect that others were using them independently. So in that respect, I would say it was a "grey zone", as we'd never advertised these classes, nor marked them as private.

I would suggest we no longer strip the following fields from the CLDR objects:

but continue stripping percentFormat, scientificFormat, units, miscPattern etc.

If this still isn't enough, then probably the best would be to keep the full CLDR files or think of splitting them (but that would bring its own complexity).

Regards, Vladi

scameron commented 1 year ago

@vladitasev, yeah, I'm not sure if it's a good idea for us to be using these utilities directly, but we didn't want to load the full Open UI5 and any other alternative would bring in their own localization files and therefore even more overhead. So this seemed like the best approach.

Looking at the code, our use of LocaleData is pretty encapsulated so I can see that the following calls are used:

Then we also use the DateFormat class where we call format and parse but I guess that's not much help because those have a lot of code paths inside them.

I'll also ask a colleague who is more familiar with this code to comment...

ac1058 commented 1 year ago

Hi @ilhan007 @vladitasev, thank you for investigating the issue so quickly. Appreciate your efforts here a lot!

@scameron Just to avoid back and forth between all of us and to ensure we are exposing the properties we need, I also went through the file for LocaleData usages and found the respective property that we need to use in the CLDR .json files. I have bolded the ones not yet captured in #6998

Here is what I've found NumberFormat.ts LocaleData.js
getPercentPattern percentFormat
getCurrencyPattern currencyFormat
getCurrencySymbol currency
getCurrencySymbols currencySymbols
getCurrencyDigits currencyDigits
getCurrencySpacing currencyFormat', 'currencySpacing'
getCustomCurrencyCodes currency
getDecimalPattern decimalFormat
getNumberSymbol (with plusSign, minusSign, decimal, group and percentSign) symbols-latn-
getLenientNumberSymbols (with plusSign and minusSign) lenient-scope-number
getUnitFromMapping unitMappings
getUnitFormat units, short
getUnitFormats units, short
getPluralCategory plurals
getCurrencyFormat currencyFormat-
getDecimalFormat decimalFormat-long, decimalFormat-short
getPluralCategories plurals

I agree that we may not want to pick and choose which properties to expose, however the current alternative is to expose the whole asset file which would balloon our webpack builds back to where they were in UI5 WC 1.10.x

Thanks!

scameron commented 1 year ago

Just for context for @vladitasev, the NumberFormat.ts module is our own wrapper class that is the place where we use LocaleData.

vladitasev commented 1 year ago

Thanks for the detailed analysis!

We reverted some more fields and merged the PR. You should be able to test the changes with the upcoming @next version

BR

ac1058 commented 1 year ago

Hi @vladitasev,

Thanks for working on this. I have just uncovered another property that is missing that we need to consume. As far as I can tell, this is the last property that is missing on our side that we need to consume.

it is for aQuartersAbbrev, which uses this.oLocaleData.getQuarters('abbreviated', sCalendarType);

Could you please help to expose this in the CLDR assets? I guess if we run into another issue after this, we should consider opening up the CLDR fields again.

Thanks, Allan