Skip to content
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

Do you know if patch is still needed #1

Open
trickeyd opened this issue Nov 9, 2021 · 5 comments
Open

Do you know if patch is still needed #1

trickeyd opened this issue Nov 9, 2021 · 5 comments

Comments

@trickeyd
Copy link

trickeyd commented Nov 9, 2021

Hey - I've been following this saga in 4/5 threads now for a couple of days and have finally come here to grab your patches.

First off - thanks for persevering and being so vocal about what you were doing, especially in the face of some pretty pigheaded responses.

I recently updated my react native app and am using hermes, and the performance has been REAL bad. Not what you would expect form over 2 years of react native updates, but i stumbled across this conversation, and we are doing a lot of date localisations.

I've added your patches and am just compiling - so hoping for an improvement (otherwise hermes is out).

I used the polyfills at the version required for your patches to work, as I see that @formatjs+ecma402-abstract+1.6.4.patch does not work for the newer versions. But that lead me to wonder if your solutions, or any other were ever added. Been trying to find relevant threads, but the trail has gone cold for me now.

Has anything changed with regard to the pollyfills, or hermes in general?

Thanks again

@andreialecu
Copy link
Owner

Hey @trickeyd 👋

I'm still using this patch because Hermes's Intl doesn't currently work properly. The timezone support in Hermes is currently broken. There's a PR to fix it here and I might try it again after it's released: facebook/hermes#627

As far as I know, there have been no fixes in formatjs for this particular issue. The maintaner has been waiting for tc39/ecma402#569 to get merged prior to resolving this.

I think your best bet is to keep using the patch for now. You probably don't need a more recent version of formatjs, I haven't kept up, but I don't think there's anything useful unlocked by upgrading.

@andreialecu
Copy link
Owner

@trickeyd I just noticed that the code in this repo isn't the full patch.

It doesn't include the bulk of the performance boost which was part of formatjs/formatjs#2827

This is the patch I use nowadays:

diff --git a/src/abstract/FormatDateTimePattern.js b/src/abstract/FormatDateTimePattern.js
index 47fd27397f6fff014af1c4b8862e26fea11e7aab..8d0d1234d9a5dcbd6131bbe206d4655a6c153726 100755
--- a/src/abstract/FormatDateTimePattern.js
+++ b/src/abstract/FormatDateTimePattern.js
@@ -32,6 +32,28 @@ function offsetToGmtString(gmtFormat, hourFormat, offsetInMs, style) {
     }
     return gmtFormat.replace('{0}', offsetStr);
 }
+var numberFormatsCache = new Map();
+function getNumberFormatsForLocale(locale, fractionalSecondDigits) {
+    var cache = numberFormatsCache.get(locale);
+    if (cache)
+        return cache;
+    var nfOptions = Object.create(null);
+    nfOptions.useGrouping = false;
+    var nf = new Intl.NumberFormat(locale, nfOptions);
+    var nf2Options = Object.create(null);
+    nf2Options.minimumIntegerDigits = 2;
+    nf2Options.useGrouping = false;
+    var nf2 = new Intl.NumberFormat(locale, nf2Options);
+    var nf3 = undefined;
+    if (fractionalSecondDigits !== undefined) {
+        var nf3Options = Object.create(null);
+        nf3Options.minimumIntegerDigits = fractionalSecondDigits;
+        nf3Options.useGrouping = false;
+        nf3 = new Intl.NumberFormat(locale, nf3Options);
+    }
+    numberFormatsCache.set(locale, [nf, nf2, nf3]);
+    return [nf, nf2, nf3];
+}
 /**
  * https://tc39.es/ecma402/#sec-partitiondatetimepattern
  * @param dtf
@@ -45,22 +67,8 @@ function FormatDateTimePattern(dtf, patternParts, x, _a) {
     var dataLocale = internalSlots.dataLocale;
     var dataLocaleData = localeData[dataLocale];
     /** IMPL END */
-    var locale = internalSlots.locale;
-    var nfOptions = Object.create(null);
-    nfOptions.useGrouping = false;
-    var nf = new Intl.NumberFormat(locale, nfOptions);
-    var nf2Options = Object.create(null);
-    nf2Options.minimumIntegerDigits = 2;
-    nf2Options.useGrouping = false;
-    var nf2 = new Intl.NumberFormat(locale, nf2Options);
     var fractionalSecondDigits = internalSlots.fractionalSecondDigits;
-    var nf3;
-    if (fractionalSecondDigits !== undefined) {
-        var nf3Options = Object.create(null);
-        nf3Options.minimumIntegerDigits = fractionalSecondDigits;
-        nf3Options.useGrouping = false;
-        nf3 = new Intl.NumberFormat(locale, nf3Options);
-    }
+    var _b = getNumberFormatsForLocale(internalSlots.locale, fractionalSecondDigits), nf = _b[0], nf2 = _b[1], nf3 = _b[2];
     var tm = ToLocalTime_1.ToLocalTime(x, 
     // @ts-ignore
     internalSlots.calendar, internalSlots.timeZone, { tzData: tzData });

I'm using it with "@formatjs/intl-datetimeformat": "patch:@formatjs/intl-datetimeformat@^4.1.5#./patches/dtf-perf.diff", with Yarn 3.1.0 via its built in patch protocol: https://yarnpkg.com/features/protocols/#patch

You should be able to adapt it to work with patch-package as well.

@trickeyd
Copy link
Author

trickeyd commented Nov 9, 2021

Hey @andreialecu 👍

Amazing! Thanks so much for getting back so quickly. I'll whack this version in instead then. Hopefully this will get me out of a pickle.

Wow - I didn't even know about yarn patch Nice one! 😄

@andreialecu
Copy link
Owner

yarn patch is only part of yarn 2 and up. If you're on yarn 1 you'll need to use patch-package. Migrating to 2+ can be a bit hard, if you attempt that, make sure you enable node_moduleslinker mode. See: https://yarnpkg.com/getting-started/migration

@trickeyd
Copy link
Author

trickeyd commented Nov 9, 2021

Ah - yes I'm on 1. I'll stick to patch-package then.

Much obliged!

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

No branches or pull requests

2 participants