kelvinhokk / cordova-plugin-localization-strings

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

Package engines node and cordova #67

Closed ath0mas closed 2 years ago

ath0mas commented 2 years ago

~I think and understand that engines for node should not be set directly here, as it is not a constraint of this package's own source code, but only from its dependencies that already define it.~

I tried to figure it out with a tool like ls-engines:

~Again I would go for this removal of engines about node.~ Indeed ls-engines suggests

Your “engines” field is either missing, or set to { "node": "*" }! Prefer explicitly setting a supported engine range.

We keep it set 👍


About "cordova", you removed it from "engines", but it is still described in Readme and declared in plugin.xml. I suggest to restore it everywhere and use cordovaDependencies available since cordova 6.1 ; why I updated the requirement from 6.0 to 6.1 here too.


I cleaned up the Readme for node and cordova (and can adjust this change depending on the choices made in this PR).

rodrigograca31 commented 2 years ago

I did remove it because it was was invalid. the correct method is in plugin.xml

Im gonna accept the PR but could you clarify whats up with --fetch? Dont the package dependencies get automatically installed? 🤔

ath0mas commented 2 years ago

About --fetch: no such option in modern cordova cli but

it existed in cordova-cli 6.x : see doc

--fetch: Fetches the plugin using npm install

was then reversed in cordova-cli 7.x : see doc

--nofetch: Do not fetch the plugin using npm install

and then removed in cordova-cli ≥ 8 with this same default npm install we know


Maybe change the line about it in Readme, to tell it's only for cordova 6 ? and to not use --nofetch in cordova 7 ? ⇒ I would personally go for simply removing it 😇