-
Notifications
You must be signed in to change notification settings - Fork 2
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
Define how localization tagging works #5
Comments
Is this specifically for translation, or are you going to also use it for general overridden data in the document? |
Proposing not using this for translation at all -- just localized content. (For example, if we want to show/hide modules on a per-locale basis.) An easy to understand example is with localized YouTube videos (i.e. same module, different YouTube video IDs on a per-locale basis). Translations would be handled with a similar tag to gettext, e.g.:
|
Ok, so it becomes an explicit localization, the template has to know that the content can be localized before it can get the localized version? In grow it is implicit since the data just changes for the template and it doesn't 'know' about the other possible data unless you manually load the yaml with a different locale. This seems like it would cause some extra code turn when you decide to add localized content. For example, if you decide to localize the videos, you would update the yaml to use the constructor, but then you would have to also update any template code that uses the video information to make it manually call the |
Agree with @Zoramite, having to add |
|
Here is a thought:
|
For the mechanics, I would suggest a |
I'm all for not having the developer specify a By the way, we should have a way to access the untranslated/unlocalized values too. In Grow.dev, developers couldn't really access the localized values of other locales within a document, without getting the entire other document. In my previous proposal, it'd be possible for a template to access the other locales without fetching the other document entirely. Example use case for translations:
In the above example we want the data attribute to be in English but the visible label to be in the translated language. |
(By the way, not blaming anyone other than myself for the performance/memory hit in Grow. That was a design that I spearheaded and I wrote the original untagging... so it was a problem I created for us.) :) |
One of the things I was going to test in Grow was having the translation be an object instead of just the translated string during untagging. So that the We could try something similar here where the |
This is a pretty clever solution if
Re: memory concerns, most memory issues are caused by keeping objects around longer than they're needed. Grow and Ama both seem to be aggressively caching YAML files for performance reasons, but I wonder if it's worth looking into turning that off in |
(Wrote this while Steven was replying...) Yeah; it's worth testing. One complication/challenge I've found is that the locale is only determined from the context... In other words, for our discussion, the I wasn't able to think of a way that the locale could both be auto-determined without access to the template rendering context's locale, as well as without having to create a copy of the YAML file for each locale that the Doc is in. But if there's a way that doesn't impact memory usage let's do it. |
Regarding the cache: I found that Node's performance with parsing YAML is incredibly fast. So we may not need the cache as it exists currently in the prototype. In my benchmarks and tests I've found the current approach works well (with up to ~10,000 documents) but you start to see slowdown after that in my simple tests. Note that the slowdown is still measured in milliseconds (i.e. 500ms) and nothing as bad as it was with Grow (i.e. 1-5+ seconds for huge sites with many locales). BTW: Node's file watcher is also really, really good (based on my tests). |
I agree, for the server, we don't really need to cache the yaml at all with how fast this seems to process the yaml files. The reason we cache so heavily in Grow is that it is a slow process for python to parse yaml. If that is no longer a concern the server can just read everything fresh and parse as needed. We may want to cache during the Re: the access to the locale, the |
Oh, the other use case for the caching--besides the multiple locale documents--is the usage of But we could be smart about that and detect which ones are referenced the most often and keep them in a cache without caching everything. Ex, the loader can track the number of times each path is loaded and the ones that start to be referenced more often (ex: > x times) can get cached instead of reread. |
Without getting too off track from the localization issue at hand, the LRU cache sounds great to me. That would help with memory usage. How do we all feel about this approach?
If the above can be done in a performant way I fully approve it. :) |
If YAML parsing is fast enough to serve requests without noticeable latency, I'd probably say removing it would simplify the architecture since a file watcher wouldn't need to be running on a child process. The file watcher, while useful, adds considerable overhead in determining when certain cache objects need to be cleared.
If the cache is needed for perf reasons, maybe using a per-request cache would be better. I'm assuming this may be tough to inject into the current codebase though since some request context would need to be added to anything that calls |
We could easily use a per request cache by passing the request object into the Pod when it's instantiated. All readYaml calls go through the Pod so they would have access to the request that way. Regarding perf, on the current prototype the dev server responds with page renders in 3-5ms. It's really fast (even for sites with 5000 pages). The only "slow" part of the system is reading all the files to produce the full list of paths. (It only gets slow after having ~5000 files each defining their own path.) But even then it creates the list in about 500ms. Also, we could require or suggest sites that are larger than X to use routes in the podspec equivalent (like Oak) and then we could use a normal trie structure like Oak and do everything lazily. I'm also OK with mandating all sites do this (even small ones) but then it would be harder for specific pages to override or customize their URL structure on a case by case basis. |
Localized the document fields when loading fields for a document. Uses the document's locale to get the correct localized value when `toString` is called. Part of #5
Localized the document fields when loading fields for a document. Uses the document's locale to get the correct localized value when `toString` is called. Related to #5 The content body part still uses null since it can be used to show that the document has been parsed, but there is no body, different from when it is not parsed and is just undefined.
We are abandoning Grow.dev's automatic localization untagging in order to limit memory usage and improve performance. After all, most content isn't localized, so it doesn't make sense to penalize all templates if content isn't localized to begin with.
We want to support the feature but require developers to use it explicitly rather than automatically.
Some thoughts:
Option 1 - Explicit
!a.Localized
YAML tag.Option 2 - Preserve the same syntax as Grow.dev
Option 3 - Something else I'm not thinking of?
I have a preference for Option 1 because the usage is clearer in templates and having the
a.Localized
object likely makes it more scalable in the future in case we need to add more features to that.The text was updated successfully, but these errors were encountered: