jeresig / i18n-node-2

Lightweight simple translation module for node.js / express.js with dynamic json storage. Uses common __('...') syntax in app and templates.
MIT License
507 stars 79 forks source link

Auto-generated locale file remembers previous keys which are no longer used. #99

Open chanpory opened 7 years ago

chanpory commented 7 years ago

Repro steps:

  1. Run this code:

    var i18n = new (require('i18n-2'))({
       locales: ['en']
    });
    
    i18n.__("foo");
    
    setTimeout(function() {
     i18n.__("bar");
    }, 30 * 1000);
  2. Observe that the generated en.js file looks like this:

    {
     "foo": "foo"
    }
  3. Delete en.js before the 30 second timeout above elapses.
  4. After the timeout elapses, observe that the generated en.js file looks like this:

    {
     "foo": "foo",
     "bar": "bar",
    }
  5. Expected result:

    {
     "bar": "bar",
    }

i18n appears to keep a representation of the localization file in memory and flush that to disk with every call. Rather, it should treat the file as the single source of truth and just patch that with each call rather than entirely recreating it. By doing the latter, it overrides the developer's intent.

wearhere commented 7 years ago

By the way, the code sample above is designed for reproducibility—the actual scenario @chanpory and I were seeing is:

  1. We call i18n.__("foo") from a template helper.
  2. We realize that we misspelled the key. We try to clean this up by removing it from the localization file and editing the template.
  3. Editing the template causes it to be rebuilt, but—crucially—in our build process, this does not require restarting our server (which would recreate the i18n object).
  4. We re-render the template (by requesting the corresponding route)—and notice that i18n has rewritten the old key into the localization file.
gjuchault commented 7 years ago

The file is not the source of the inside object at any time. That means removing or editing the file will not update the actual object inside i18n. Files are cached and all that stuff. You need to remove the key yourself from the inside object using delete

wearhere commented 7 years ago

Thanks for the quick response @gjuchault.

The file is not the source of the inside object at any time.

I guessed as much, I am saying that is problematic. :) The source of truth should be the file, at least locally—surely caching is not necessary then—to simplify editing. In this scenario there was no opportunity to call delete, we were just editing HTML templates not JS. And if we had edited JS then the server would have restarted and i18n's internal state would have been purged anyway.

gjuchault commented 7 years ago

The source of truth should be the file

The file will not be read every time you get a value. Caching is necessary. I didn't get the part where you say you don't have any JS possibilty

wearhere commented 7 years ago

The file will not be read every time you get a value. Caching is necessary.

Could you explain why this needs be the case locally, as opposed to in production?

It'd be more complex, but I could also envision a system where caching continued to be employed locally, yet the file was the source of truth—using chokidar or something similar to watch the file and reload i18n's internal state from the file when the latter changed.

I didn't get the part where you say you don't have any JS possibilty

In our scenario, at no time did we edit any JS. We only edited the HTML template and the localization file.

It's actually generally unclear to me where you would call delete to clean up individual keys like in our scenario. You'd have to write a one-off delete call only to remove it as soon as the key was gone. And when you restarted the server to make use of the delete call, it would purge i18n's internal state—so there wouldn't even be any more need for the delete call, if you just deleted the line from the localization file. Much easier to just edit the localization file.

gjuchault commented 7 years ago

Could you explain why this is the case locally? [and next paragraph]

You're not supposed to edit a generated file by your library to change the way it works while it's on. The library generates this file.

Furthermore, reading a file is a costly operation, compared to a object getter. Using a file watcher would solve a problem that is yours and not i18n-node-2's (see the previous point).

In your scenario, you make a mistake and then you want to correct it and make i18n-node-2 refresh its cache by using a file watcher. This looks kinda weird. I'm used to reload my server whenever I make a change inside, or use something like nodemon or others (there are plenty) to make it reload when there is a change. Did you consider using a process watcher (even a complex one as ps2 to manage more properly reloads) ?

wearhere commented 7 years ago

You're not supposed to edit a generated file by your library to change the way it works while it's on. The library generates this file.

Let's back up. How are users of i18n-node-2 supposed to remove localizations from the file, other than editing the file? There is no "remove" or "delete" API. But it wouldn't even make sense to use such API since such API calls would be one-offs, removed immediately after they were executed.

Right?

gjuchault commented 7 years ago

Do you think this issue is somehow related to what you mean https://github.com/jeresig/i18n-node-2/issues/97 ?

wearhere commented 7 years ago

Hm, maybe? I could describe what I'm looking for as syncing i18n-node-2's cache with the file. @kokujin, what do you think?