-
Notifications
You must be signed in to change notification settings - Fork 647
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
Intl.NumberFormat notation: compact is not working on 0.71.7 #1035
Comments
Hey @AndreiCalazans thank you for reporting this. Are you observing this on iOS or Android? In general, our Intl APIs are implemented on top of APIs exposed by the underlying platform, so there are gaps where we cannot map cleanly onto the underlying platform. |
For android it is documented on the website that using the compact option has some "rough edges". It says nothing about iOS tough. Saying it has "rough edges" cloud maybe be more specific as it this case it seems to mean not making it compact at all. Edit: |
@jobpaardekooper Thanks for trying it out. Building the CLI on macOS should be good enough for developing for macOS and iOS, since the APIs available are mostly the same. The only thing to be careful about is that we can't use ICU, because access to the system ICU may be allowed on macOS, but is blocked on iOS. For Android, the only way currently is to build an app, since the implementation is in Java and uses Android APIs. We'd welcome discussions of potential implementations, and a PR. As a starting point, you can take a look at PlatformIntlApple.mm for macOS development, since that should be the most convenient to start trying things out. |
Thank you for pointing me in the right direction! Will start looking into the code a bit more and will try to make a fix. From what I could find there are no tests for this specific functionality. Should that be added or is it fine like it is? If it should be added do you maybe have some tips/resources to get started for writing the tests? Or a document/file where I can find info about the testing for Intl. Also another question that kind of confused me. When running
But Intl only seems to be working when compiled in with the Again, thanks a lot for the help. |
Regarding testing, we currently run against test262, which is the primary source of tests for both Android and iOS Intl implementations. You can take a look at our CI set-up here to see how it is done. We also have some additional tests here that you can add to if you add features. Note that these are primarily for running on macOS. Intl is enabled by default when you build with it, so you shouldn't need to touch |
@neildhar I was looking into it a little bit and found the following comment in PlatformIntlApple.mm:
All this iOS stuff is implemented using Apple's NSNumberFormatter. But there is no NSNumberFormatter that supports these cases listed in the comment above so I am assuming that is why they have not been implemented. I don't think this would be easy to do seeing as we can't use ICU and Apple does not have support for it. Maybe you have any ideas on how this could be achieved? Additionally, the "Internationalization APIs" page in the docs does not mention that these options are not supported. It actually specifically mentions that the format function is supported according to the standard and then continues to list a few limitations for android. I think these iOS limitations should also be added to the docs just like the android limitations are there. Do you have any idea how this could feasibly be implemented? Because this example of using 'compact' varies heavily for each language. If not I can create a PR to add this information to the docs. |
I do not unfortunately. I remember looking into these when we first rolled out
We would welcome such a PR. As you have observed, the page currently only talks about Android, and we should document the iOS support as well. |
@neildhar I have created the PR. Maybe you can review it? |
Summary: As discussed in issue #1035, the documentation of the Intl APIs does not include the current state of the iOS support. This PR adds this information and formats the page to reflect that this page now talks about Android and iOS support and not just Android. Also the "Not yet supported" sections has been removed. The page now only mentions what **is** supported and the limitations of the functionalities that are currently supported. This makes more sense with regard to the still changing specification. Mentioning what is supported automatically means the remaining information from the spec that is not included under "supported" is not currently supported. Pull Request resolved: #1042 Reviewed By: avp Differential Revision: D47155521 Pulled By: neildhar fbshipit-source-id: c30c51008b734d922aeb5856e805642a3657a8c0
Hey, I was away for the past weeks. I caught this issue on iOS and I did not test on Android. @jobpaardekooper I see that you started documenting the limitations, did you test compact works on Android too? ps: thanks @jobpaardekooper for investigating this ❤️ |
@AndreiCalazans I did not test it but in the docs at the Android 11 section it says notation: 'compact' has some rough edges on Android. |
Summary: Original Author: [email protected] Original Git: 28e1a33 As discussed in issue #1035, the documentation of the Intl APIs does not include the current state of the iOS support. This PR adds this information and formats the page to reflect that this page now talks about Android and iOS support and not just Android. Also the "Not yet supported" sections has been removed. The page now only mentions what **is** supported and the limitations of the functionalities that are currently supported. This makes more sense with regard to the still changing specification. Mentioning what is supported automatically means the remaining information from the spec that is not included under "supported" is not currently supported. Original Reviewed By: avp Original Revision: D47155521 Reviewed By: neildhar Differential Revision: D47690636 fbshipit-source-id: 4829c12b2ff4fdfda4a5eeb07d3cb569154799c0
@neildhar As we had discussed a while ago,
There are two major issues with this though.
I can imagine there might be a work around possible regarding the Swift only thing. But, I think iOS 15+ is a complete deal breaker right? Or would there be some way to optionally enable this? |
is there a workaround for this? doesn't work on android and ios for me |
@jobpaardekooper We could conditionally compile it out on older iOS versions, but this wouldn't help in general until RN's minimum iOS version is past 15+. (except for users who are willing to build Hermes from source) |
Unfortunately for us we were forced to polyfill with |
We did as well... but it is insanely slow (I use the formatter on a list with a lot of elements). It would be very nice to see a native implementation both on iOS and Android. maybe it can be polyfilled on the native side? |
@jgo80 double check that formatjs ins't using Hermes' native Intl APIs since they are available formatjs won't oveerride them. We had to patch shouldPolyfill calls within formatjs to always return true. Hermes' native Intl APIs are very slow. |
@AndreiCalazans Thx. Even with |
FYI, this is our plan for dealing with Intl problems: #1211 |
Is there a workaround for this? |
Ok as a workaround you can use this function:
|
I made this function to convert my numbers, to continue using Hermes.
|
Bug Description
gradle clean
and confirmed this bug does not occur with JSCHermes version: {
"Bytecode Version": 90,
"Builtins Frozen": false,
"VM Experiments": 0,
"Build": "Release",
"GC": "hades (concurrent)",
"OSS Release Version": "for RN 0.71.7",
"CommonJS Modules": "None"
}
React Native version (if any): 0.71.7"
OS version (if any): 0.71.7"
Platform (most likely one of arm64-v8a, armeabi-v7a, x86, x86_64): all
Steps To Reproduce
code example:
The Expected Behavior
Expected:
The text was updated successfully, but these errors were encountered: