Skip to content
This repository has been archived by the owner on Oct 1, 2024. It is now read-only.

Use @shopify/dates formatDate function and memoize Intl.NumberFormat #1287

Merged
merged 1 commit into from
Feb 25, 2020

Conversation

helloneele
Copy link
Contributor

@helloneele helloneele commented Feb 18, 2020

Description

Previous PRs: #1277, #1286

This makes use of the now exported formatDate function from the @shopify/dates package.

It also changes Intl.NumberFormat.format() to be memoized on SSR. This is because it is, same as Intl.DateTimeFormat.format(), leaking memory.

You can test this by doing node --inspect leak.js using node v10 or v12

// leak.js

setInterval(() => {
  var b;
  for (let i = 0; i < 100; i++) {
    b = new Intl.NumberFormat("ja-JP", {
      style: "currency",
      currency: "JPY"
    }).format(Math.floor(Math.random() * Math.floor(1000000)));
  }
}, 1);

Right after start
image

After a minute
image

I opened an issue in the node bug tracker to hopefully get the V8 fix backported to node v10/v12.

Type of change

  • Patch: Bug/ Documentation fix (non-breaking change which fixes an issue or adds documentation)
  • Minor: New feature (non-breaking change which adds functionality)
  • Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have added a changelog entry, prefixed by the type of change noted above

@helloneele helloneele mentioned this pull request Feb 18, 2020
4 tasks
@helloneele helloneele force-pushed the reacti18n-use-formatDate-from-dates-pkg branch 3 times, most recently from 4baa588 to 141005c Compare February 19, 2020 19:48
@helloneele helloneele deployed to beta-test-reacti18n-memory-leak-fix February 20, 2020 20:41 Active
@helloneele helloneele changed the title Use @shopify/dates formatDate function instead of custom one Use @shopify/dates formatDate function and memoize Intl.NumberFormat Feb 24, 2020
@helloneele helloneele force-pushed the reacti18n-use-formatDate-from-dates-pkg branch from 81322e6 to 5b1729d Compare February 24, 2020 15:38
@helloneele helloneele requested a review from michenly February 24, 2020 15:43
@helloneele helloneele force-pushed the reacti18n-use-formatDate-from-dates-pkg branch from 5b1729d to fdd4d85 Compare February 24, 2020 17:25
@@ -17,6 +17,17 @@ const MISSING_TRANSLATION = Symbol('Missing translation');
const PLURALIZATION_KEY_NAME = 'count';
const SEPARATOR = '.';

const numberFormats = new Map();
export const memoizedNumberFormatter = function(locale, options = {}) {
const key = numberFormatCacheKey(locale, options);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because we are using this at at least two places now, I think we should just add an enabledOnServer option to memoize. But that can also be done in a follow up PR.

@helloneele helloneele force-pushed the reacti18n-use-formatDate-from-dates-pkg branch from fdd4d85 to 0569d36 Compare February 25, 2020 14:44
@helloneele helloneele merged commit 0d30b0e into master Feb 25, 2020
@helloneele helloneele deleted the reacti18n-use-formatDate-from-dates-pkg branch February 25, 2020 14:58
@carysmills carysmills deployed to beta-test-formatting-12-hr-dates February 26, 2020 19:32 Active
@michenly michenly temporarily deployed to production February 27, 2020 20:05 Inactive
@helloneele helloneele deployed to test-use-memoized-number-format March 6, 2020 19:25 Active
@TayKangSheng TayKangSheng deployed to expand_locale_support_for_shopify_address March 9, 2020 08:16 Active
@michenly michenly temporarily deployed to gem March 24, 2020 21:43 Inactive
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants