Skip to content
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

Some improvements and retryInDefaultLocale option #206

Closed
wants to merge 6 commits into from

Conversation

Paxa
Copy link

@Paxa Paxa commented Feb 12, 2016

Make Mustache as optional dependency

It will require mustache only if your key has {{}} in it, you can override rendering method via i18n._renderMustach(msg, namedValues)

Related: #113

Move all options in i18n.options, make it accessible from outside

I know some people like to keep things in private variables (What for? idk), but I hate when I need to fork library just to change some little thing inside. With this changes you can read/write options one by one.

// Example
i18n.options.defaultLocale // -> 'en'
i18n.options.fallbacks['ms'] = 'id'; 
i18n.options.fallbacks['uk'] = 'ru'; 
if (app.get('env') === 'production') {
  i18n.options.updateFiles = false;
}

Expose API to override serialization:

I often put tailing coma in locale file and it gets revamped, with this I can set it in JS:
This feature will let you change format of locale file, it can be anything: yaml, js, properties, xml...

i18n.serializeLocale(localeObj)  // -> convert object to string
i18n.deserializeLocale(fileContent) // -> parse file content to object

// override deserializer to parse it as JS instead of JSON
i18n.deserializeLocale = function (fileContent) {
  return eval('var a = ' + fileContent + '; a');
};

Related: #79 #73

Retry with retryInDefaultLocale option

With it's on, i18n will use default locale if can't find translation in current locale. I'm planing to use it in production. If I forget to add some translation - I prefer to see english words in production instead of section.something.like.this

i18n.configure({
  ...
  retryInDefaultLocale: true // default is false
});

// let's say english locale has this key, but user's locale don't
i18n.__('sidebar.promo.title') // -> "We have promo!!!"

P.S. I don't insist to merge it, but can be useful for other people

i18n.serializeLocale(localeObj) - convert object to string
i18n.deserializeLocale(fileContent) - parse file content to object
@mashpie
Copy link
Owner

mashpie commented Feb 13, 2016

Hi,
some very great thoughts! Each of them is on my roadmap in one or another way. Unfortunately your PR makes it more complicated to merge as needed. You combined 4 features/improvements in that one PR and only one of them comes with dedicated test-coverage, which makes it a bit unsafe to merge the whole PR.

If you don't mind, I'd take care of splitting and merging in a separated feature-branch or you could submit 4 PRs instead of one big one. Now, some words concerning each of those features:

  • retryInDefaultLocale: Excacly. Special use case of fallbacks, but I was thinking of a different configuration like fallbacks {'*':'en', 'at':'de'} to get even more controll
  • override serialization: nice for now, will be part of a "storage backend" release where I'd like to open the whole thing to submodules and add some defaults
  • i18n.options: Ok, I am also tired of all those "private globals" that evolved with more and more config options. But I must say that I don't like the idea of exposing options as simple object. Things we need to cover is: "What'll happen when I change directory on runtime? Decline? Reload? Copy?" so I am sure we'll need to expose an options object with dedicated getter/setter methods that can react on changes that will probably affect the whole "static" setup done earlier with i18n.configure() - readability, maintainability of code would be much better though. Thus I am thinking of refactoring that into a separated submodule. So your suggestion is a nice half way to that, but needs some work after merge.
  • decrease dependency: mustache - Yes, I hate depenecies :) And it is a very good idea to keep that optional. Still needs some testing and dev friendly messaging: "You need to npm install mustache --save in order to use mustache placeholders"

So all in all I had to battle with my fingers not to click on "merge pull request" - but for now I have to focus on that get|setLocale/register config option. You might leave that PR open and I'll split it or you might close and send separated PRs. Thank you very much in either way :)

@Paxa
Copy link
Author

Paxa commented Feb 13, 2016

Well explained, thanks.

  • For retryInDefaultLocale do you want to have same configuration style as fallbacks? I can try to implement it, will '*' : 'en' override defaultLocale setting? Do you have better name option, it's hard to remember even for me :)
  • about mustache - agree,
  • i18n.options - haven't think about changing that on a fly, what options do you think not save just change? directory, prefix, extension, autoReload? I was also thinking, should i18n.configure reset all properties or only specified once. Now it will re-assign all options

I add test for serialization and add message if mustaches modules is missing.

Will it be easier for you just take parts parts that you like and improve if needed?

@mashpie mashpie added this to the 0.9.0 - feature improvements milestone Feb 20, 2016
@isayme
Copy link

isayme commented May 10, 2016

@mashpie is there a plan to release 0.9.0 ?

@mashpie
Copy link
Owner

mashpie commented May 10, 2016

When its done :) will be in weeks

Von meinem iPhone gesendet

Am 10.05.2016 um 08:37 schrieb iSayme [email protected]:

@mashpie is there a plan to release 0.9.0 ?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@eloquence
Copy link

Note that as currently implemented this will cause infinite recursion for non-existent messages, because the accessor() check will just keep failing. I've implemented a naive improvement here:
eloquence@b2e67a8

@derN3rd
Copy link

derN3rd commented Mar 7, 2018

Any news on that?

Would love to have the retryOnDefaultFallback feature in production

@Mizzick
Copy link

Mizzick commented Jul 23, 2019

Waiting for this

@mashpie mashpie removed this from the 0.9.0 - feature improvements milestone Aug 2, 2020
mashpie added a commit that referenced this pull request Aug 19, 2020
@mashpie mashpie added project tracked in a project and removed feature request labels Aug 19, 2020
@mashpie
Copy link
Owner

mashpie commented Aug 19, 2020

Hey @Paxa

as written earlier it is hard to merge multiple features in at once, so I decided to take your proposals and review & implement them one-by-one. So this started today with the famous long wanted retryInDefaultLocale feature

  • Add retryInDefaultLocale

  • Make Mustache as optional dependency

  • Refactor i18n.options

  • Add API to override serialization (storage)

Others will follow in separate releases.

Thanks again for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project tracked in a project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants