apache / cordova-plugman

Apache Cordova Plugman
https://cordova.apache.org/
Apache License 2.0
400 stars 151 forks source link

Fix platform_name option #112

Closed raphinesse closed 5 years ago

raphinesse commented 5 years ago

Fixes #110

erisu commented 5 years ago

@raphinesse There is one question I had on something I ran into.

The command blow works as expected.

$ npx plugman install --platform ios --project ./ --plugin cordova-plugin-camera
Installing "cordova-plugin-camera" for ios

Is it expected for --platform and --platform_name is an interchangeable argument?

For example, should this command work?

$ npx plugman install --platform_name ios --project ./ --plugin cordova-plugin-camera

Currently the command above will fail.

Per the help printout from plugman, it appears that the platform_name argument is strictly for plugman platform add and plugman platform remove. If this is expected then I am OK with the PR.

raphinesse commented 5 years ago

@erisu Thanks for the review.

It is like you state in your last paragraph. The platform_name option is only used in the platform sub-command.

elmorgan3 commented 5 years ago

Hello, I installed that plugin on Monday, but it didn't work, until I saw that change on this pull request. But there are not published yet, in npm. I had to change it, to make it work. I don't know if you know it, but I wanted to communicate it.

raphinesse commented 5 years ago

@elmorgan3 Yes, this fix is not released yet. Since our release process is a bit time consuming, I was hoping to get a few other fixes (#113) merged before we start preparing a patch release.

monica18p commented 5 years ago

@elmorgan3

Hello, I installed that plugin on Monday, but it didn't work, until I saw that change on this pull request. But there are not published yet, in npm. I had to change it, to make it work. I don't know if you know it, but I wanted to communicate it.

I had to change it, to make it work

What did you change to make it work?

elmorgan3 commented 5 years ago

Hi @monica18p, you need to modify main.js after install plugaman, with the code there are on last pull-request done https://github.com/raphinesse/cordova-plugman/blob/cfe3dfc8b84410a94483488dcfa92746e514ba19/main.js. In my computer the plugin is installed on root "AppData/Roaming/npm/node_modules/plugman/main.js" I hope that's help you.

raphinesse commented 5 years ago

Installing from master would probably be an easier way to test the fix: npm i -g apache/cordova-plugman.

Disclaimer: Note that you will install a development version that could be unstable.

monica18p commented 5 years ago

@raphinesse Noted. Can you please suggest any stable solution?

@elmorgan3 After updating main.js, package.json is being created a little weird. it takes id as "true" by its own

{
  "name": "cordovaPluginXXX",
  "version": "1.0.0",
  "description": "XXX plugin",
  "cordova": {
    "id": "true",
    "platforms": [
      "ios",
      "android"
    ]
  },
  "keywords": [
    "ecosystem:cordova",
    "cordova-ios",
    "cordova-android"
  ],
  "author": "XXX",
  "license": "ISC"
}

Did you encounter something similar?

raphinesse commented 5 years ago

@monica18p The "stable" solution would be waiting for the next official release. In the meantime, I can give you my personal opinion that right now, master is probably more stable than the current release.

I believe that the second bug you encountered should be fixed by #113 (more specifically https://github.com/apache/cordova-plugman/commit/6410354ec558b9dd98243e3c4dc6630598dcd6f9) once that is merged and released. If you want to verify that your problem is fixed by that PR, you can install that version via npm i -g raphinesse/cordova-plugman#fix-orgy for testing purposes.

monica18p commented 5 years ago

But master is not able to add platforms.. For that need a stable solution.. but never mind will keep a watch on new release,, for now I have updated main.js as suggested by @elmorgan3

Thanks.