fix(exporter): fix cache overwriting changes to translation files

[xDisfigure]

fixes #78

Add caching abilities to check if :

• translation file as change and must be updated in cache (based on last modified time)
• if the old version does not equal the actual we update the translation file (base on originalTranslationFile & translationFile variable)

Context

Environment: development Tool:

• Webpack (with watch option)
• babel-loader
• babel-plugin-i18next-extract

Expected

Our problem is the plugin re-write the translation file using the version it had at build time.

Example:

locale.json

{
  "foo": ""
}

build -> the plugin has the key 'foo' and the value ''

Inside locale.json I change the value of foo with easy and I save the file.

locale.json

{
  "foo": "easy"
}

I add a key in my view named 'toto', the plugin extract that new key and add it inside our translation file.

theoretically locale.json should look like this

{
  "foo": "easy",
  "toto": ""
}

but the plugin keeps the old version (version at the build time) and I got this:

{
   "foo": "",
   "toto": ""
}

Fixes

I have added :

• a check on the mtime of the file being processed
• a check to prevent from writing the translation file if it equals the old version

• [x] I have added unit test for those cases
• [ ] documentation (if needed)

[gilbsgilbs]

Thanks a lot for this PR. It looks complicated but is an interesting fix to this annoying issue :thinking: . From user's perspective, I think the behavior implemented in this PR is more correct or at least more expected.

However, given that the exporter code is fairly complex already (because of how I had to bend babel's plugin API), I'm not so sure that I'm confident merging this fix as is, unless I can convince myself that it is indeed our only way, and most importantly that it doesn't introduce new undesirable side-effects (which seems hard to predict at this point IMO).

Another approach that I think would be a lot easier to implement while working around the issue in 99% of cases and being a lot less "risky" would be to implement a flag (babel option and/or environment variable) that adds a dry-run mode (i.e. prevents writes to outputs). I'd agree that it is technically less correct (as you loose live-reloads and stuff compared to what's done in this PR), but I'd feel a bit more confident shipping this. WDYT?

Also, one of the test cases introduced by this PR is failing for Node 10 :disappointed: .

[xDisfigure]

Thanks a lot for this PR. It looks complicated but is an interesting fix to this annoying issue 🤔 . From user's perspective, I think the behavior implemented in this PR is more correct or at least more expected.

However, given that the exporter code is fairly complex already (because of how I had to bend babel's plugin API), I'm not so sure that I'm confident merging this fix as is, unless I can convince myself that it is indeed our only way, and most importantly that it doesn't introduce new undesirable side-effects (which seems hard to predict at this point IMO).

Another approach that I think would be a lot easier to implement while working around the issue in 99% of cases and being a lot less "risky" would be to implement a flag (babel option and/or environment variable) that adds a dry-run mode (i.e. prevents writes to outputs). I'd agree that it is technically less correct (as you loose live-reloads and stuff compared to what's done in this PR), but I'd feel a bit more confident shipping this. WDYT?

Also, one of the test cases introduced by this PR is failing for Node 10 😞 .

@gilbsgilbs I think I should remove the part with the deepEqual (to prevent translation file to be written if it does not change), because its not the actual problem here. I should remove it from that pull request and think about creating a new one later If really needed.

Removing that part should drastically lower the risk of side-effects.

The other fix should not be a problem if the one above is removed. It actually just compare two date to invalidate the cache of the translation file and reload it from disk.

I can add an option to the config file to support this "last modified" check

I'd like to have your feedback about this possible solution

I'll take a look about that node10 unit test which fail and try to make more robust unit test. I'll detail the flow to better understand how the code is executed and see if there are any potential side-effects.

About your second approach, It does not solve our actual problem and we'll loose tons of useful things. I prefer to work a bit more on that solution than having the half solution :)

[sakulstra]

I'd agree with @xDisfigure that the proposed alternative has to many drawbacks over what is proposed here so I'd rather see this move forward.

I'm not so sure that I'm confident merging this fix

I guess removing the deepEqual at first would be a good idea, if this doesn't resolve @gilbsgilbs concerns perhaps we could release this as beta/experimental to not expose to many users to the altered behavior at once?

[gilbsgilbs]

@xDisfigure @sakulstra I completely understand your points. Let's not go for the workaround I suggested then :+1: .

TBH, the deep equal comparison does not concern me much. I'm just not sure JSON.stringify is guaranteed to be consistent across runs according to ECMA, but if not we could just use stable-stringify and live with it. Even performance-wise, I think the overhead is very negligible in comparison to the I/Os and traversing the whole AST like this plugin does.

The "last modification time" however is system dependent and possibly overridden by CVS or any userland program. It looks weak and unreliable to me and at the end of the day, it's hard to figure out if that's really the behavior we intend and if it achieves what we really expect it to achieve. That's why I am not very enthusiastic about it.

What about the radically different approach of not using stats or deep equal to compare files, but rather using some library like deepmerge to reload the local file before the exporter is called:

// instead of
if (!(filePath in cache.originalTranslationFiles)) {
  cache.originalTranslationFiles[filePath] = loadTranslationFile(...);
}

// do
cache.originalTranslationFiles[filePath] = deepmerge(
  cache.originalTranslationFiles[filePath] ?? {},
  loadTranslationFile(...),
);

// or maybe deepmerge "cache.currentTranslationFiles[filePath]" depending on what makes more sense.

Would that work? It looks like a more systematic and less "if-y" approach to the issue, so I'd say it could be less prone to corner cases. But I definitely may be plain wrong (as I'm well aware deep-merging also can lead to funny jokes and I'm not even sure it works fully).

[xDisfigure]

@xDisfigure @sakulstra I completely understand your points. Let's not go for the workaround I suggested then 👍 .

TBH, the deep equal comparison does not concern me much. I'm just not sure JSON.stringify is guaranteed to be consistent across runs according to ECMA, but if not we could just use stable-stringify and live with it. Even performance-wise, I think the overhead is very negligible in comparison to the I/Os and traversing the whole AST like this plugin does.

The "last modification time" however is system dependent and possibly overridden by CVS or any userland program. It looks weak and unreliable to me and at the end of the day, it's hard to figure out if that's really the behavior we intend and if it achieves what we really expect it to achieve. That's why I am not very enthusiastic about it.

What about the radically different approach of not using stats or deep equal to compare files, but rather using some library like deepmerge to reload the local file before the exporter is called:

// instead of
if (!(filePath in cache.originalTranslationFiles)) {
  cache.originalTranslationFiles[filePath] = loadTranslationFile(...);
}

// do
cache.originalTranslationFiles[filePath] = deepmerge(
  cache.originalTranslationFiles[filePath] ?? {},
  loadTranslationFile(...),
);

// or maybe deepmerge "cache.currentTranslationFiles[filePath]" depending on what makes more sense.

Would that work? It looks like a more systematic and less "if-y" approach to the issue, so I'd say it could be less prone to corner cases. But I definitely may be plain wrong (as I'm well aware deep-merging also can lead to funny jokes and I'm not even sure it works fully).

Indeed this approach will solve our "stale translation file" problem. Implementing it

EDIT: Done. It works as expected and seems to be easier to understand and maintain. cc @sakulstra

I have added deepmerge as dependency because it is a popular and well maintained package. It avoid us to maintain and test the deepmerge part in this repository. Let me know if you're good with that.

I have 1 added only one test to check if the translation file is well reloaded from disk and merged with cache. @gilbsgilbs If you think about a unit test case I missed feel free to report it.

[gilbsgilbs]

Thanks :1st_place_medal: . I'll try to make a release today.