Cache support

[rxaviers]

Requires:

• Runtime code (smaller and quicker) for production #398

[jzaefferer]

Might help to set this PR against #436 to only show the cache changes.

[jzaefferer]

I think last time we talked about runtime optimization and cache support, we discussed making the caching part of the runtime optimizer, to avoid having to sprinkle it all over the code (like you do here now). Did you investigate that option?

[rxaviers]

Might help to set this PR against #436 to only show the cache changes.

It would have been. I'm afraid I'd have to create a new PR by now to do that, right?

[rxaviers]

I think last time we talked about runtime optimization and cache support, we discussed making the caching part of the runtime optimizer, to avoid having to sprinkle it all over the code (like you do here now). Did you investigate that option?

Yes and it's handled there. This PR adds cache support for the normal library. The benefit would be making this transparent for users.

[jzaefferer]

In that case I don't think this is worth the additional complexity. Anyone unwilling to apply the full runtime optimization can trivially cache the formatter in their own code.

[rxaviers]

Anyone unwilling to apply the full runtime optimization can trivially cache the formatter in their own code.

@dpolivy has shown interest for this feature. So, I'd like to hear if he has any thoughts.

[dpolivy]

@rxaviers @jzaefferer I haven't followed the runtime PR too closely, but I guess my main question is, would that be easy to incorporate into a node.js environment? At a high level it makes sense, but it also looks a lot more complicated than npm install globalize cldr-data and require('globalize').

As an aside, I've actually seen ~2-3 second startup time increase in our node app since adding in i18n support with globalize (mostly loading all the CLDR JSON), so any possible optimizations would be great to incorporate (especially if they include caching).

Here's how I'm doing my own caching in express-globalize (a library I'm working on for integrating with express). It's serviceable, but really I think the library should handle this automatically. The main challenge is just consistently "hashing" the options for each formatter to avoid duplication. I'm not sure JSON.stringify is ideal as it might not account for different ordering of parameters, but I wanted something that would be super cheap to calculate (otherwise you lose the benefit of caching in the first place).

[rxaviers]

As an aside, I've actually seen ~2-3 second startup time increase in our node app since adding in i18n support with globalize (mostly loading all the CLDR JSON), so any possible optimizations would be great to incorporate (especially if they include caching).

#398 will reduce significantly your load time, not this PR though.

The main challenge is just consistently "hashing" the options for each formatter to avoid duplication

This is exactly what this PR does. The hash it uses is here https://github.com/jquery/globalize/pull/470/files#diff-981bf4d78b64521bd698f83454bb3cf9

[dpolivy]

Anyone unwilling to apply the full runtime optimization can trivially cache the formatter in their own code.

@jzaefferer While true at a high level, I'd argue that the hashing of the options object isn't necessarily trivial, and as such warrants the addition of caching within the library. I can't really think of any (user) scenario here where you wouldn't want the library to be optimized in this way.

[jzaefferer]

@rxaviers with this additional cache, wouldn't the cache that the optimizer adds end up being redundant? Or does the optimizer optimize this cache away?

@dpolivy thanks for the details on your usecase, that helps a lot. Based on that I agree that this feature is worth it.

[rxaviers]

@jzaefferer your second statement: it's not an additional cache, but the same one that optimizer adds up.

[rxaviers]

Cache is useful to avoid creating a plural generator for every message formatter #817.

[rxaviers]

I am closing all old (and stalled) PRs

[dpolivy]

@rxaviers Are there plans to eventually add full caching support as discussed here? There's still value in the feature.

[rxaviers]

Yes, I think so @dpolivy. Ideally this feature is optional and can be injected when desired.