kelvinhokk / cordova-plugin-localization-strings

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

Fix attempting to add strings for platforms which are not built now #25

Closed shelart closed 5 years ago

shelart commented 6 years ago

I have Android SDK on Windows but the app which I'm building is cross-platform iOS+Android. iOS builds are generated by another machine. My Cordova config.xml contains, of course, two platforms.

Currently the plugin (imho) incorrectly handles this case. When I try to cordova build android on Windows machine it crashes with UnhandledPromiseRejectionWarning #<Object>. This is because the plugin attempts to locate iOS project dir but fails.

This little PR fixes the issue.

rodrigograca31 commented 6 years ago

only changing the way you get platforms is enough to fix this? interesting...... thought you would need to check if the platform actually exists or something...

rodrigograca31 commented 5 years ago

This PR has extra changes that shouldnt be there... like the package lock file....

rodrigograca31 commented 5 years ago

Thanks for your contribution but I think I will make the changes manually later.

pklaes commented 5 years ago

This really should be an one liner, changing context.requireCordovaModule('cordova-lib/src/cordova/util').listPlatforms(context.opts.projectRoot) to context.opts.platforms in create_strings.js.

Have a look at This sample in the docs

And yes, probably easier to do it manually. I did so on my test environment and it worked.

shelart commented 5 years ago

Thank you @pklaes, I'm happy this does work not only for me :)

shelart commented 5 years ago

I'll clean up PR with only one-liner change tomorrow, so @rodrigograca31 you'll be able just merge it into without conflicts.

rodrigograca31 commented 5 years ago

@pklaes yeah, I thought about changing that like but the deprecation notice said it was only for non cordova modules..... I guessed cordova/util was a cordova module so didnt mess with it.

I also didnt do it because I got confused.... The docs context object example has ctx.opts.cordova.platforms which is an array. but then the docs use ctx.opts.platforms.includes('android') .... Note no cordova object....

Also, maybe its not a one-liner....? cause if its an array then indexOf wont work.....?

https://github.com/kelvinhokk/cordova-plugin-localization-strings/blob/6d8b54c69545da332cd59175c4497aa481842464/scripts/create_strings.js#L10-L16

@shelart Thanks, I will be waiting for your PR

shelart commented 5 years ago

@shelart Thanks, I will be waiting for your PR

Did that.

rodrigograca31 commented 5 years ago

@shelart thanks for your contribution! :heart: