-
Notifications
You must be signed in to change notification settings - Fork 106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dependencies and format #64
Dependencies and format #64
Conversation
Looks good! Thanks and thanks for testing! |
I will merge and publish as 4.0.0 as soon as I solve an issue with my npm account which isnt letting me publish... 😭 |
I will have some more cleanup and minor rewrites to suggest later today ; in this PR or a new one after this one merged? |
I think its fine if you keep it in this one. like I said I will review and publish everything in the end as 4.0.0 |
Done for me - ready for review |
@@ -196,7 +196,7 @@ function processResult(context, lang, langJson, stringXmlJson) { | |||
_.forEach(langJsonToProcess, function (val, key) { | |||
// positional string format is in Mac OS X format. change to android format | |||
val = val.replace(/\$@/gi, '$s'); | |||
val = val.replace(/\'/gi, "\\'"); | |||
val = val.replace(/'/gi, "\\'"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope this doesnt break stuff. probably wont but I dont know regex well enought. 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My IDE detected it as a "Redundant character escape in RegExp" and indeed quotes are not reserved characters.
if (platforms.indexOf('android') >= 0) { | ||
promises.push(android_script(context)); | ||
} | ||
|
||
if (platforms.indexOf('ios') >= 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure why this re-order was necessary ... 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary but alpha order as much as common order in Cordova plugins to deal with Android before iOS.
package.json
Outdated
@@ -29,7 +29,6 @@ | |||
"glob": "^7.2.0", | |||
"iconv-lite": "^0.6.3", | |||
"lodash": "^4.17.21", | |||
"path": "^0.12.7", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 less dependency! yay! 🥳 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! :) and more after all ; I am going to edit PR description to give detail about what changed here with all those newer commits added.
scripts/create_ios_strings.js
Outdated
if (err) throw err; | ||
fs.writeFileSync(fd, buffer); | ||
}); | ||
fs.writeFileSync(path.join(lProjPath, fileName), stringToWrite); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering exactly this! I didnt understand why there was encoding going on.... thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that, before #35, the encoding used to write file was 'utf16' (requiring iconv-lite and buffer).
Back to using 'utf8' means we can simply use writeFile
👍 .
package.json
Outdated
@@ -29,7 +29,6 @@ | |||
"glob": "^7.2.0", | |||
"lodash": "^4.17.21", | |||
"xcode": "^3.0.1", | |||
"xml-writer": "^1.7.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another one!? 😮 great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one was never required nor used yes
Let me ask you for just one last "fix" seems its used to use You did a great job so far! The community really appreciates it! |
:) I saw it too yesterday, only basic But I went back to it today as you asked for: |
I will now tag it as 4.0.0 but cant push to npm yet.... |
(will now add |
Great! Thanks a lot, guys! |
More dependencies update (fix #63): correct version in package-lock, xcode v3, @xmldom/xmldom
+ Some formatting mostly to cleanup js whitespaces and indents
Used and everything is fine for both android and ios in my case 👍
(you should toggle "Hide whitespace" or look at each commit separately for easier review)
Dependencies:
fs-extra
: replaced by nodefs
+ smaller single-responsibilitymkdirp
iconv-lite
: removed, node 'utf8' default encoding to write files is finelodash
: replaced byunderscore
, same api, smaller bundlexcode
: updated from v0.9 to v3xml-writer
removed, unusedxml2js
: okxmldom
: removed, unique use replaced by regex matchingpath
: removed, nodepath
is fineglob
: okStill OK for both android and ios in my test project.
Maybe add a line inside Readme about "new" node version "compatibility" in 4.0.0? reviewing package-lock.json
"engines"
may help.=>
"node": ">=10"
formkdirp
andxcode
v3