feat: add ability to localize common characters #208
Conversation
dlockhart
force-pushed
the
GAUD-7171/localize-common-characters
branch
from
ed36b4d
to
5c5d537
Compare
If more than one collection is fetched, does that mean they are effectively multiple copies of it in memory? It's probably not going to be a huge set, but doesn't seem ideal. Maybe there is a way to share one set? Or, maybe there could be a way for those components to indicate whether to include the common terms in the set? I'm not too familiar with how the terms are fetched, so maybe none of those things are possible.
I think async |
| @@ -164,8 +191,15 @@ export const getLocalizeClass = (superclass = class {}) => class LocalizeClass e | |||
| } | |||
| if (Object.prototype.hasOwnProperty.call(this, 'getLocalizeResources') || Object.prototype.hasOwnProperty.call(this, 'resources')) { | |||
| const possibleLanguages = this._generatePossibleLanguages(config); | |||
| const resourcesPromise = this.getLocalizeResources(possibleLanguages, config); | |||
| resourcesLoadedPromises.push(resourcesPromise); | |||
| if (config?.importFunc) { | |||
There was a problem hiding this comment.
I had to add this check to handle the case where the consumer only wants to use the common resources.
| const resourcesPromise = this.getLocalizeResources(possibleLanguages, config); | ||
| resourcesLoadedPromises.push(resourcesPromise); | ||
| } | ||
| if (config?.loadCommon) { |
There was a problem hiding this comment.
This is now only loading the common resources if the consumer explicitly asks for it.
I think that might be true currently, but we should be able to shift things around a bit to avoid that. The set might actually grow quite a bit so we should be sure about that. Otherwise this all looks really good to me. |
| importFunc: async(lang) => { | ||
| if (commonResources.has(lang)) return commonResources.get(lang); | ||
| const resources = (await import(`../lang/${lang}.js`)).default; | ||
| commonResourcesImportCount++; |
There was a problem hiding this comment.
This now keeps a local cache of the common resources, which prevents multiple in-memory copies of the same resources for a given language.
Testing that this was actually working was tricky since we can't observe whether a dynamic import happens or not. I went with a simple counter, which is maybe a bit fragile but gets the job done.
There was a problem hiding this comment.
This is good for now, I think, but the problem is that we make copies of everything by combining them into a single object per-element, which is bad. We should make a separate story to address this before we expand the list of common messages.
dlockhart
force-pushed
the
GAUD-7171/localize-common-characters
branch
from
a3ae484
to
148db7e
Compare
I had some trouble figuring out how best to integrate this, so definitely interested in feedback and other ideas.
The big downside to this approach is that every component that wires up to a localizer via the
LocalizeMixinis going to have these "common" terms merged into their collection. That's why I needed to use theintl-commonprefix on all their keys to avoid a potential collission.It also means that the common terms will always be fetched once on every page, even if they're not used. BothThat's no longer true now thatlocalizeandlocalizeHTMLare synchronous, so I couldn't think of a way to defer the loading of the common stuff.loadCommon: truemust be set in the config.