kelvinhokk / cordova-plugin-localization-strings

Cordova Plugin for Localization of Strings on the App
MIT License
119 stars 107 forks source link

Version 5.0.0 - 5.0.2 regex broken on Windows #83

Closed fordkilleen closed 1 year ago

fordkilleen commented 1 year ago

Hi,

Today I noticed an issue where new builds were not getting the translations added to the strings.xml files and did some digging, finding it to be related to the regex and the updated glob dependency. Hope you don't mind I've put up a PR for this as otherwise the plugin does not work on Windows for versions 5.0.0 to 5.0.2.

Issue: Since version 5.0.0 of this plugin, the regex used to get the language from the json files to then create the strings.xml files was not working. This was due to the update of the glob dependency (now 10.2.1) which has had some changes regarding forward and back slashes since the previously used version (7.2.0).

Example: glob would return something like my-path\en.json but the regex being used was /my-path\/(.*).json Effectively meaning the regex was expecting a / but on Windows would get \ path separator.

Steps to reproduce:

  1. Start with a clean cordova repository, or clean out an existing one (remove platform and plugins folders).
  2. Run a cordova build which will create the Android platform folder and included resources
  3. You will see on versions 5.0.0 and 5.0.2 that the translations from your en.json, es.json, etc. never made it into the strings xml file or had folders created (eg. res/values-es/)

Fix: This broken regex issue (which causes this plugin to not work at all on Windows) is easily resolved by using a newly added glob option posix: true, which forces it to return / delimited paths on Windows instead of \, which matches the expected regex already in the repo.

PR has been raised: #82

Ref: Lots of changes with slashes and separators in glob between 7.2.0 and 10.2.1 https://github.com/isaacs/node-glob/blob/main/changelog.md

rodrigograca31 commented 1 year ago

Thanks man!

This is why I love open source! and why I released 5.0.0 in case stuff broke.

and yeah I tested it but I'm on Linux.... :sweat_smile:

its merged, now im gonna do the new release stuff....

rodrigograca31 commented 1 year ago

Done. Its in 5.0.4

fordkilleen commented 1 year ago

Awesome, thanks for the quick turnaround! 💪

peitschie commented 1 year ago

@rodrigograca31 just in case it's of any interest, I've started using GitHub actions to help out with this kind of cross-platform testing for another cordova-plugin: https://github.com/don/cordova-plugin-ble-central/blob/master/.github/workflows/cordova.yml

I wonder how hard it would be to add some tests here to run as part of an action🤔

peitschie commented 1 year ago

I'd be happy to spend a bit of time helping out with this if you have an approach or idea in mind @rodrigograca31 about how you'd like to confirm the output...

rodrigograca31 commented 1 year ago

That sounds like a good idea! let me make a new issue for it