ionic-team / ionic-app-lib

The library used for using ionic apps - consumed by the CLI and the GUI
44 stars 79 forks source link

State.savePlugin fails to write variables if only one is provided. #25

Open BennettSmith opened 9 years ago

BennettSmith commented 9 years ago

If a plugin is added with a single variable it will not be properly saved into the package.json file.

For example, if this command is run:

$ ionic plugin add https://github.com/meetfm/com.speedshare.cordova.video.git --variable VIDEOKIT_DIR=/tmp

you will see something like this added to the package.json file:

...
    {
      "locator": "https://github.com/meetfm/com.speedshare.cordova.video.git",
      "id": "com.speedshare.cordova.video",
      "variables": {
        "": ""
      }
    }
...

What is expected is something like this:

...
    {
      "locator": "https://github.com/meetfm/com.speedshare.cordova.video.git",
      "id": "com.speedshare.cordova.video",
      "variables": {
        "VIDEOKIT_DIR": "/tmp"
      }
    }
...

If more than one variable is included then the output is as expected.

talamaska commented 9 years ago

it actually doesn't save any variables. I have just tested. there are some strange thing going on in the state.js Why in the function saveExistingPlugins if the plugin has variable you check for feature params? Why hasVariables is reset to false. This is provoking issues. I noticed one more thing - when installing the facebook plugin from npm and try to save it while adding it doesn't add the variables in the package.json.

the reason is around https://github.com/driftyco/ionic-app-lib/blob/master/lib/state.js#L620 Surprisingly the logic misses adding locator id and variables if the plugin is not local aka "./myplugin folder". the logic from https://github.com/driftyco/ionic-app-lib/blob/master/lib/state.js#L627 https://github.com/driftyco/ionic-app-lib/blob/master/lib/state.js#L628 https://github.com/driftyco/ionic-app-lib/blob/master/lib/state.js#L637 to https://github.com/driftyco/ionic-app-lib/blob/master/lib/state.js#L642

should be repeated in the above condition. also the getPluginFromFetchJsonByLocator will not find the correct locator from fetch.json

https://github.com/driftyco/ionic-app-lib/blob/master/lib/state.js#L700

in order to make it work I'm adding

hasVariables = Object.keys(lookupPlugin.variables).length !== 0;

    if ((isNotRegistyPlugin && hasUrlOrPathOfLocator) || (isNotRegistyPlugin && hasPathWithLeadingDot) || (!isNotRegistyPlugin && !hasUrlOrPathOfLocator && !hasPathWithLeadingDot && hasVariables)) {
BennettSmith commented 9 years ago

It sounds like you may be running into a different issue than I was. Could you provide an example of the command-line you used when no variables were saved?

talamaska commented 9 years ago

ionic plugin add phonegap-facebook-plugin --variable APP_ID="xxxxxxxx" --variable APP_NAME="yyyyy" --save

talamaska commented 9 years ago

Currently trying to figure out how to make ionic state save work with variables values. I'm not succeeding yet

talamaska commented 9 years ago

I don't get it , why it looks for the feature objects instead of the plugins to get the values of the parameters

BennettSmith commented 9 years ago

Try leaving off the --save option. Does it write the variables to package.json then?

talamaska commented 9 years ago

nope

BennettSmith commented 9 years ago

Could you post your entire package.json file? I am guessing that there is some other difference in how you are using the CLI commands. When I added a plugin with variables it always saved correctly (if I had more than one variable).

talamaska commented 9 years ago

btw I'm on Windows 8.1 if this matter

{
  "name": "xxxx",
  "version": "x.y.z",
  "description": "xxxx: A Ionic project",
  "scripts": {
    "postinstall": "bower install"
  },
  "dependencies": {
    "child-process-promise": "^1.1.0",
    "gulp": "^3.5.6",
    "gulp-cheerio": "^0.6.2",
    "gulp-concat": "^2.2.0",
    "gulp-dom-src": "^0.2.0",
    "gulp-flatten": "0.0.4",
    "gulp-html2js": "^0.2.0",
    "gulp-minify-css": "^0.3.0",
    "gulp-ng-annotate": "^1.0.0",
    "gulp-prettify": "^0.3.0",
    "gulp-rename": "^1.2.0",
    "gulp-replace": "^0.5.3",
    "gulp-ruby-sass": "^1.0.5",
    "gulp-uglify": "^1.2.0",
    "run-sequence": "^1.1.1"
  },
  "devDependencies": {
    "bower": "^1.3.3",
    "gulp-util": "^2.2.14",
    "shelljs": "^0.3.0"
  },
  "cordovaPlugins": [
    "cordova-plugin-whitelist",
    "cordova-plugin-crosswalk-webview",
    "cordova-plugin-device",
    "cordova-plugin-file-transfer",
    "cordova-plugin-file",
    "cordova-plugin-network-information",
    "cordova-plugin-geolocation",
    "cordova-plugin-statusbar",
    "cordova-plugin-splashscreen",
    "cordova-plugin-camera",
    "ionic-plugin-keyboard"
  ],
  "cordovaPlatforms": [
    {
      "platform": "android",
      "version": "",
      "locator": "android"
    }
  ]
}
jbavari commented 8 years ago

Hey all! This is patched in master. We'll release it sometime soon. Thanks for the feedback.