gilbsgilbs / babel-plugin-i18next-extract

Babel plugin that statically extracts i18next and react-i18next translation keys.
https://i18next-extract.netlify.com
MIT License
161 stars 37 forks source link

Fix empty transpiled file cache issue #227

Closed xu3u4 closed 2 years ago

xu3u4 commented 2 years ago

To fix #208 I found the exporterCache object doesn't persist across all the runs in newer babel versions(7.16.10+). After some runs, it becomes empty. The solution is inspired by https://github.com/babel/babel/discussions/13590#discussioncomment-1034864 It points out that the cache object needs to be put outside the plugin function. (maybe we can also consider to use weakMap for the performance improvement.)

post() {
      const extractState = this.I18NextExtract;
      if (extractState.extractedKeys.length === 0) return;
      console.log("exporterCache")
      console.log(exporterCache)
      // ... rest
},
Before After
image image

Btw, thank you for the awesome plugin, it really helps our i18n flow a lot by reducing many manual steps! 🌟 🌟 🌟

xu3u4 commented 2 years ago

@gilbsgilbs Please check it when you have time 🙏

gilbsgilbs commented 2 years ago

Thanks for your contribution.

maybe we can also consider to use weakMap for the performance improvement

I'm not sure I understand this part. What would be the keys of such weakMap? Instances of BabelCore.ConfigAPI? If that works, I think I'd prefer that over plain globals, but I suspect babel wont even pass consistent instances of ConfigAPI across runs 😕 . It's not much about performance though.

Also, do you know at which point this broke? Or was it always broken somehow?

Did you try with the latest release candidate + completely removing and recreating your lockfile and node_modules?

ITJesse commented 2 years ago

I can confirm that this patch works great. Without this, the output will always be the last file. This problem is also reported here: #208

gilbsgilbs commented 2 years ago

I just checked, and the api object indeed changes across calls (so I really don't understand how a weakmap is supposed to help here). I think the most sensible thing to do would be to create a cache per package.json (by using "root" or finding the closest to the source file using pkg-up or something) and fallback to a global cache only in last resort. But given nobody may actually have a use-case for this and as discardOldKeys is just broken at the moment, I'll consider this out of scope for this PR.

Added a commit with miscellaneous improvements not really related to your PR while I was playing around.

Thanks for your contribution @xu3u4 🙏 .

gilbsgilbs commented 2 years ago

Published under 0.9.0-rc.1 . I'll make a final release at some point.

xu3u4 commented 2 years ago

Thank you very much! 🙏
Sorry for the late reply. I was busy with other stuff and haven't got time to check it carefully. Looking forward for the new release!

so I really don't understand how a weakmap is supposed to help here

Sorry it may not fit this case as the keys are string instead of object. But I guess the suggestion is trying to say that weakMap element can be garbage collected if the key is removed.