Fix memory leak in @shopify/dates when SSR

[helloneele]

Description

This fixes the memory leak in the dates package that we were seeing when upgrading to any version after v 0.1.26 in Web.

More info is here: https://github.com/Shopify/web/issues/23454

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

[marutypes]

Why does this not cause leaks but the implementation from function-enhancers does? Could we fix it there so that we don't risk this happening again in the future?

[helloneele]

The function enhancers implementation doesn't memoize when server side rendering. https://github.com/Shopify/quilt/blob/master/packages/function-enhancers/src/memoize.ts#L18

[helloneele]

I don't know what the reasoning was behind not memoizing when SSRing, I mostly just wanted to try this out if that fixes things. It does locally, but wanted to see it in production. Happy to change this later on. Hence my question for a beta release.

[michenly]

SSR means the react app only render the one time.
This mean memoize on server actually does not enhance performance, but leave behind memozing that will never be clear off.

this is the reasoning in theory, but maybe in practice there are some thing we are missing?

[marutypes]

In the past we've had memory leaks from memoizing on the server, so that was the idea between not doing so. I fear this might make things worse instead of better.

[marutypes]

That said, if it does work locally I suppose it's worth a shot. It worries me that we have no real rationale for why it would work though :(

[helloneele]

Explanation for not memoizing on the server makes sense. 👍

It worries me that we have no real rationale for why it would work though :(

I think the rationale for this is that there is a v8/Node.js issue which leads to Intl.DateTimeFormat not being properly cleaned up by the GC, and since we don't memoize we create more and more of those objects. So memoizing those objects minimizes the overall amount of objects we'll create which ends up in less unreachable objects for the GC.

[michenly]

if we have legit reasons to not skip over server memorization (which I assume could happen else where), then lets add an option to memoize to specifically memorize no matter what.

export default function memoize<Method extends (...args: any[]) => any>(
  method: Method,
  resolver?: (...args: Parameters<Method>) => any,
  skipOnServer = true
): Method {

if (skipOnServer && typeof window === 'undefined') {
      return method.apply(this, args);
    }

[helloneele]

I'll try what I have now in production, if that works I'm happy to change the memoizer. 👍
Though, I still think this is a very rare case just related to Intl.DateTimeFormat.