Details

    • New Feature
    • Status: Closed
    • Major
    • Resolution: Implemented
    • None
    • None
    • None

    Description

      As per "Proposal: hooks support for plugins" dev mail thread discussion

      Hi, I have an idea how we can add more flexibility to plugin developers.

      Note, right now we have Application Developers – someone who use Cordova for developing applications and Plugin Developers – someone who creates plugins so that Application Developers can use them. For Application Developers we expose hooks so that they can customize their build/package/etc process. I want us to provide similar sort of flexibility to Plugin Developers so that they can go beyond of <source/>, <framework/> tags and get mechanism to add custom installation, build logic required by a plugin. Example usage will include: downloading/compiling additional binaries, marking source file to be copied to output dir, changing target build platform, etc. At present time the steps described could be only achieved by hooks manually added by Application Developer, but the right way is to allow Plugin Developer to expose this as part of plugin definition.

      Example configuration could look like
      ```
      <script type="postinstall" src="scripts/postinstall.js" />
      <script type="preinstall" src="scripts/preinstall.js" />
      <script type="install" src="scripts/install.js" />
      ```
      beforeinstall/preinstall – run before plugin is installed
      install/postinstall/afterinstall – run after plugin is installed
      uninstall – run after plugin is uninstalled

      Attachments

        Issue Links

        Activity

          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on the pull request:

          https://github.com/apache/cordova-plugman/pull/74#issuecomment-40968416

          Created issue CB-6481 to cover this work.
          Switched to nodejs module loader.
          Sample hook file implementation with async functionality:

          ```
          var Q = require('./q');

          module.exports = function(platform, projectDir, pluginDir, cmdLine) {
          console.log('hook.js: ' + platform);
          console.log('hook.js: ' + projectDir);
          console.log('hook.js: ' + pluginDir);
          console.log('hook.js: ' + cmdLine);

          var deferral = new Q.defer();

          setTimeout(function()

          { deferral.resolve(); }

          , 1000);

          return deferral.promise;
          }
          ```

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on the pull request: https://github.com/apache/cordova-plugman/pull/74#issuecomment-40968416 Created issue CB-6481 to cover this work. Switched to nodejs module loader. Sample hook file implementation with async functionality: ``` var Q = require('./q'); module.exports = function(platform, projectDir, pluginDir, cmdLine) { console.log('hook.js: ' + platform); console.log('hook.js: ' + projectDir); console.log('hook.js: ' + pluginDir); console.log('hook.js: ' + cmdLine); var deferral = new Q.defer(); setTimeout(function() { deferral.resolve(); } , 1000); return deferral.promise; } ```
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on the pull request:

          https://github.com/apache/cordova-plugman/pull/74#issuecomment-41850345

          Switched to single parameter called context

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on the pull request: https://github.com/apache/cordova-plugman/pull/74#issuecomment-41850345 Switched to single parameter called context
          githubbot ASF GitHub Bot added a comment -

          Github user agrieve commented on a diff in the pull request:

          https://github.com/apache/cordova-plugman/pull/74#discussion_r12193116

          — Diff: src/util/hooks.js —
          @@ -0,0 +1,124 @@
          +/*
          + * Copyright (c) Microsoft Open Technologies, Inc.
          — End diff –

          Don't think it should say this.

          githubbot ASF GitHub Bot added a comment - Github user agrieve commented on a diff in the pull request: https://github.com/apache/cordova-plugman/pull/74#discussion_r12193116 — Diff: src/util/hooks.js — @@ -0,0 +1,124 @@ +/* + * Copyright (c) Microsoft Open Technologies, Inc. — End diff – Don't think it should say this.
          githubbot ASF GitHub Bot added a comment -

          Github user agrieve commented on a diff in the pull request:

          https://github.com/apache/cordova-plugman/pull/74#discussion_r12193146

          — Diff: src/util/hooks.js —
          @@ -0,0 +1,124 @@
          +/*
          + * Copyright (c) Microsoft Open Technologies, Inc.
          + *
          + * Licensed under the Apache License, Version 2.0 (the "License");
          + * you may not use this file except in compliance with the License.
          + * You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing,
          + * software distributed under the License is distributed on an
          + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
          + * KIND, either express or implied. See the License for the
          + * specific language governing permissions and limitations
          + * under the License.
          + */
          +
          +var path = require('path'),
          + fs = require('fs'),
          + child_process = require('child_process'),
          + Q = require('q'),
          + events = require('../events'),
          + os = require('os'),
          + isWindows = (os.platform().substr(0,3) === 'win');
          +
          +/**
          + * Implements functionality to run plugin level hooks.
          + * Hooks are defined in plugin config file as <script> elements.
          + */
          +module.exports = {
          + /**
          + * Fires specific plugin hook: 'preinstall', 'afterinstall', 'uninstall', etc.
          + * Async. Returns promise.
          + */
          + fire : function(type, plugin_id, pluginElement, platform, project_dir, plugin_dir) {
          + // args check
          + if (!type)

          { + throw Error('hook type is not specified'); + }

          +
          + events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.');
          +
          + var scriptTypes = getScriptTypesForHook(type);
          + if (scriptTypes == null)

          { + throw Error('unknown plugin hook type: "' + type + '"' ); + }

          +
          + var scriptFilesToRun = getScriptFiles(pluginElement, scriptTypes, platform);
          + context = {
          — End diff –

          Missing "var". This is the main reason I think using commas & multi-line var statements is a bad idea.

          githubbot ASF GitHub Bot added a comment - Github user agrieve commented on a diff in the pull request: https://github.com/apache/cordova-plugman/pull/74#discussion_r12193146 — Diff: src/util/hooks.js — @@ -0,0 +1,124 @@ +/* + * Copyright (c) Microsoft Open Technologies, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +var path = require('path'), + fs = require('fs'), + child_process = require('child_process'), + Q = require('q'), + events = require('../events'), + os = require('os'), + isWindows = (os.platform().substr(0,3) === 'win'); + +/** + * Implements functionality to run plugin level hooks. + * Hooks are defined in plugin config file as <script> elements. + */ +module.exports = { + /** + * Fires specific plugin hook: 'preinstall', 'afterinstall', 'uninstall', etc. + * Async. Returns promise. + */ + fire : function(type, plugin_id, pluginElement, platform, project_dir, plugin_dir) { + // args check + if (!type) { + throw Error('hook type is not specified'); + } + + events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.'); + + var scriptTypes = getScriptTypesForHook(type); + if (scriptTypes == null) { + throw Error('unknown plugin hook type: "' + type + '"' ); + } + + var scriptFilesToRun = getScriptFiles(pluginElement, scriptTypes, platform); + context = { — End diff – Missing "var". This is the main reason I think using commas & multi-line var statements is a bad idea.
          githubbot ASF GitHub Bot added a comment -

          Github user agrieve commented on a diff in the pull request:

          https://github.com/apache/cordova-plugman/pull/74#discussion_r12193181

          — Diff: src/util/hooks.js —
          @@ -0,0 +1,124 @@
          +/*
          + * Copyright (c) Microsoft Open Technologies, Inc.
          + *
          + * Licensed under the Apache License, Version 2.0 (the "License");
          + * you may not use this file except in compliance with the License.
          + * You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing,
          + * software distributed under the License is distributed on an
          + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
          + * KIND, either express or implied. See the License for the
          + * specific language governing permissions and limitations
          + * under the License.
          + */
          +
          +var path = require('path'),
          + fs = require('fs'),
          + child_process = require('child_process'),
          + Q = require('q'),
          + events = require('../events'),
          + os = require('os'),
          + isWindows = (os.platform().substr(0,3) === 'win');
          +
          +/**
          + * Implements functionality to run plugin level hooks.
          + * Hooks are defined in plugin config file as <script> elements.
          + */
          +module.exports = {
          + /**
          + * Fires specific plugin hook: 'preinstall', 'afterinstall', 'uninstall', etc.
          + * Async. Returns promise.
          + */
          + fire : function(type, plugin_id, pluginElement, platform, project_dir, plugin_dir) {
          + // args check
          + if (!type)

          { + throw Error('hook type is not specified'); + }

          +
          + events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.');
          +
          + var scriptTypes = getScriptTypesForHook(type);
          + if (scriptTypes == null)

          { + throw Error('unknown plugin hook type: "' + type + '"' ); + }

          +
          + var scriptFilesToRun = getScriptFiles(pluginElement, scriptTypes, platform);
          + context =

          { + platform: platform, + projectDir: project_dir, + pluginDir: plugin_dir, + cmdLine: process.argv.join(' '), + pluginId: plugin_id + }

          ;
          +
          + return runScripts(scriptFilesToRun, context);
          + }
          +};
          +
          +/**
          + * Returns all script types represented corresponding hook event.
          + * Allows to use multiple script types for the same hook event (usage simplicity),
          + * For example:
          + * <script type='install' .. /> or <script type='postinstall' .. /> could be used
          + * to define 'afterinstall' hook.
          + */
          +function getScriptTypesForHook(hookType) {
          + var knownTypes =

          { + beforeinstall: ['beforeinstall', 'preinstall'], + afterinstall: ['install', 'afterinstall', 'postinstall'], + uninstall: ['uninstall'] + }

          +
          + return knownTypes[hookType.toLowerCase()];
          +}
          +
          +/**
          + * Gets all scripts from the plugin xml with the specified types.
          + */
          +function getScriptFiles(pluginElement, scriptTypes, platform) {
          + var scriptFiles= [],
          + scriptElements = pluginElement.findall('./script').concat(
          + pluginElement.findall('./platform[@name="' + platform + '"]/script'));
          +
          + var pendingScripts = [];
          +
          + scriptElements.forEach(function(script){
          + if (script.attrib.type && scriptTypes.indexOf(script.attrib.type.toLowerCase()) > -1 && script.attrib.src)

          { + scriptFiles.push(script.attrib.src); + }

          + });
          + return scriptFiles;
          +}
          +
          +/**
          + * Async runs the script files.
          + */
          +function runScripts(scripts, context) {
          + var pendingScripts = [];
          +
          + scripts.forEach(function(script){
          + pendingScripts.push(runScriptFile(script, context));
          — End diff –

          This runs all scripts at the same time. Probably want to run them serially.

          githubbot ASF GitHub Bot added a comment - Github user agrieve commented on a diff in the pull request: https://github.com/apache/cordova-plugman/pull/74#discussion_r12193181 — Diff: src/util/hooks.js — @@ -0,0 +1,124 @@ +/* + * Copyright (c) Microsoft Open Technologies, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +var path = require('path'), + fs = require('fs'), + child_process = require('child_process'), + Q = require('q'), + events = require('../events'), + os = require('os'), + isWindows = (os.platform().substr(0,3) === 'win'); + +/** + * Implements functionality to run plugin level hooks. + * Hooks are defined in plugin config file as <script> elements. + */ +module.exports = { + /** + * Fires specific plugin hook: 'preinstall', 'afterinstall', 'uninstall', etc. + * Async. Returns promise. + */ + fire : function(type, plugin_id, pluginElement, platform, project_dir, plugin_dir) { + // args check + if (!type) { + throw Error('hook type is not specified'); + } + + events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.'); + + var scriptTypes = getScriptTypesForHook(type); + if (scriptTypes == null) { + throw Error('unknown plugin hook type: "' + type + '"' ); + } + + var scriptFilesToRun = getScriptFiles(pluginElement, scriptTypes, platform); + context = { + platform: platform, + projectDir: project_dir, + pluginDir: plugin_dir, + cmdLine: process.argv.join(' '), + pluginId: plugin_id + } ; + + return runScripts(scriptFilesToRun, context); + } +}; + +/** + * Returns all script types represented corresponding hook event. + * Allows to use multiple script types for the same hook event (usage simplicity), + * For example: + * <script type='install' .. /> or <script type='postinstall' .. /> could be used + * to define 'afterinstall' hook. + */ +function getScriptTypesForHook(hookType) { + var knownTypes = { + beforeinstall: ['beforeinstall', 'preinstall'], + afterinstall: ['install', 'afterinstall', 'postinstall'], + uninstall: ['uninstall'] + } + + return knownTypes [hookType.toLowerCase()] ; +} + +/** + * Gets all scripts from the plugin xml with the specified types. + */ +function getScriptFiles(pluginElement, scriptTypes, platform) { + var scriptFiles= [], + scriptElements = pluginElement.findall('./script').concat( + pluginElement.findall('./platform [@name="' + platform + '"] /script')); + + var pendingScripts = []; + + scriptElements.forEach(function(script){ + if (script.attrib.type && scriptTypes.indexOf(script.attrib.type.toLowerCase()) > -1 && script.attrib.src) { + scriptFiles.push(script.attrib.src); + } + }); + return scriptFiles; +} + +/** + * Async runs the script files. + */ +function runScripts(scripts, context) { + var pendingScripts = []; + + scripts.forEach(function(script){ + pendingScripts.push(runScriptFile(script, context)); — End diff – This runs all scripts at the same time. Probably want to run them serially.
          githubbot ASF GitHub Bot added a comment -

          Github user agrieve commented on a diff in the pull request:

          https://github.com/apache/cordova-plugman/pull/74#discussion_r12193231

          — Diff: src/util/hooks.js —
          @@ -0,0 +1,124 @@
          +/*
          + * Copyright (c) Microsoft Open Technologies, Inc.
          + *
          + * Licensed under the Apache License, Version 2.0 (the "License");
          + * you may not use this file except in compliance with the License.
          + * You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing,
          + * software distributed under the License is distributed on an
          + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
          + * KIND, either express or implied. See the License for the
          + * specific language governing permissions and limitations
          + * under the License.
          + */
          +
          +var path = require('path'),
          + fs = require('fs'),
          + child_process = require('child_process'),
          + Q = require('q'),
          + events = require('../events'),
          + os = require('os'),
          + isWindows = (os.platform().substr(0,3) === 'win');
          +
          +/**
          + * Implements functionality to run plugin level hooks.
          + * Hooks are defined in plugin config file as <script> elements.
          + */
          +module.exports = {
          + /**
          + * Fires specific plugin hook: 'preinstall', 'afterinstall', 'uninstall', etc.
          + * Async. Returns promise.
          + */
          + fire : function(type, plugin_id, pluginElement, platform, project_dir, plugin_dir) {
          + // args check
          + if (!type)

          { + throw Error('hook type is not specified'); + }

          +
          + events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.');
          +
          + var scriptTypes = getScriptTypesForHook(type);
          + if (scriptTypes == null)

          { + throw Error('unknown plugin hook type: "' + type + '"' ); + }

          +
          + var scriptFilesToRun = getScriptFiles(pluginElement, scriptTypes, platform);
          + context =

          { + platform: platform, + projectDir: project_dir, + pluginDir: plugin_dir, + cmdLine: process.argv.join(' '), + pluginId: plugin_id + }

          ;
          +
          + return runScripts(scriptFilesToRun, context);
          + }
          +};
          +
          +/**
          + * Returns all script types represented corresponding hook event.
          + * Allows to use multiple script types for the same hook event (usage simplicity),
          + * For example:
          + * <script type='install' .. /> or <script type='postinstall' .. /> could be used
          + * to define 'afterinstall' hook.
          + */
          +function getScriptTypesForHook(hookType) {
          + var knownTypes =

          { + beforeinstall: ['beforeinstall', 'preinstall'], + afterinstall: ['install', 'afterinstall', 'postinstall'], + uninstall: ['uninstall'] + }

          +
          + return knownTypes[hookType.toLowerCase()];
          +}
          +
          +/**
          + * Gets all scripts from the plugin xml with the specified types.
          + */
          +function getScriptFiles(pluginElement, scriptTypes, platform) {
          + var scriptFiles= [],
          + scriptElements = pluginElement.findall('./script').concat(
          + pluginElement.findall('./platform[@name="' + platform + '"]/script'));
          +
          + var pendingScripts = [];
          +
          + scriptElements.forEach(function(script){
          + if (script.attrib.type && scriptTypes.indexOf(script.attrib.type.toLowerCase()) > -1 && script.attrib.src)

          { + scriptFiles.push(script.attrib.src); + }

          + });
          + return scriptFiles;
          +}
          +
          +/**
          + * Async runs the script files.
          + */
          +function runScripts(scripts, context) {
          + var pendingScripts = [];
          +
          + scripts.forEach(function(script)

          { + pendingScripts.push(runScriptFile(script, context)); + }

          );
          +
          + return Q.all(pendingScripts);
          +};
          +
          +/**
          + * Async runs single script file.
          + */
          +function runScriptFile(scriptPath, context) {
          +
          + scriptPath = path.join(context.pluginDir, scriptPath);
          +
          + if(!fs.existsSync(scriptPath))

          { + events.emit('warn', "Script file does't exist and will be skipped: " + scriptPath); + return Q(); + }

          + var scriptFn = require(scriptPath);
          +
          + // if hook is async it can return promise instance and we will handle it
          + return Q(new scriptFn(context));
          — End diff –

          why "new" it?

          githubbot ASF GitHub Bot added a comment - Github user agrieve commented on a diff in the pull request: https://github.com/apache/cordova-plugman/pull/74#discussion_r12193231 — Diff: src/util/hooks.js — @@ -0,0 +1,124 @@ +/* + * Copyright (c) Microsoft Open Technologies, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +var path = require('path'), + fs = require('fs'), + child_process = require('child_process'), + Q = require('q'), + events = require('../events'), + os = require('os'), + isWindows = (os.platform().substr(0,3) === 'win'); + +/** + * Implements functionality to run plugin level hooks. + * Hooks are defined in plugin config file as <script> elements. + */ +module.exports = { + /** + * Fires specific plugin hook: 'preinstall', 'afterinstall', 'uninstall', etc. + * Async. Returns promise. + */ + fire : function(type, plugin_id, pluginElement, platform, project_dir, plugin_dir) { + // args check + if (!type) { + throw Error('hook type is not specified'); + } + + events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.'); + + var scriptTypes = getScriptTypesForHook(type); + if (scriptTypes == null) { + throw Error('unknown plugin hook type: "' + type + '"' ); + } + + var scriptFilesToRun = getScriptFiles(pluginElement, scriptTypes, platform); + context = { + platform: platform, + projectDir: project_dir, + pluginDir: plugin_dir, + cmdLine: process.argv.join(' '), + pluginId: plugin_id + } ; + + return runScripts(scriptFilesToRun, context); + } +}; + +/** + * Returns all script types represented corresponding hook event. + * Allows to use multiple script types for the same hook event (usage simplicity), + * For example: + * <script type='install' .. /> or <script type='postinstall' .. /> could be used + * to define 'afterinstall' hook. + */ +function getScriptTypesForHook(hookType) { + var knownTypes = { + beforeinstall: ['beforeinstall', 'preinstall'], + afterinstall: ['install', 'afterinstall', 'postinstall'], + uninstall: ['uninstall'] + } + + return knownTypes [hookType.toLowerCase()] ; +} + +/** + * Gets all scripts from the plugin xml with the specified types. + */ +function getScriptFiles(pluginElement, scriptTypes, platform) { + var scriptFiles= [], + scriptElements = pluginElement.findall('./script').concat( + pluginElement.findall('./platform [@name="' + platform + '"] /script')); + + var pendingScripts = []; + + scriptElements.forEach(function(script){ + if (script.attrib.type && scriptTypes.indexOf(script.attrib.type.toLowerCase()) > -1 && script.attrib.src) { + scriptFiles.push(script.attrib.src); + } + }); + return scriptFiles; +} + +/** + * Async runs the script files. + */ +function runScripts(scripts, context) { + var pendingScripts = []; + + scripts.forEach(function(script) { + pendingScripts.push(runScriptFile(script, context)); + } ); + + return Q.all(pendingScripts); +}; + +/** + * Async runs single script file. + */ +function runScriptFile(scriptPath, context) { + + scriptPath = path.join(context.pluginDir, scriptPath); + + if(!fs.existsSync(scriptPath)) { + events.emit('warn', "Script file does't exist and will be skipped: " + scriptPath); + return Q(); + } + var scriptFn = require(scriptPath); + + // if hook is async it can return promise instance and we will handle it + return Q(new scriptFn(context)); — End diff – why "new" it?
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on the pull request:

          https://github.com/apache/cordova-plugman/pull/74#issuecomment-41967514

          @agrieve, thx for review; updated the code. Also did rebase

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on the pull request: https://github.com/apache/cordova-plugman/pull/74#issuecomment-41967514 @agrieve, thx for review; updated the code. Also did rebase
          githubbot ASF GitHub Bot added a comment -

          Github user ligaz commented on a diff in the pull request:

          https://github.com/apache/cordova-plugman/pull/74#discussion_r12255211

          — Diff: src/util/hooks.js —
          @@ -0,0 +1,148 @@
          +/**
          + Licensed to the Apache Software Foundation (ASF) under one
          + or more contributor license agreements. See the NOTICE file
          + distributed with this work for additional information
          + regarding copyright ownership. The ASF licenses this file
          + to you under the Apache License, Version 2.0 (the
          + "License"); you may not use this file except in compliance
          + with the License. You may obtain a copy of the License at
          +
          + http://www.apache.org/licenses/LICENSE-2.0
          +
          + Unless required by applicable law or agreed to in writing,
          + software distributed under the License is distributed on an
          + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
          + KIND, either express or implied. See the License for the
          + specific language governing permissions and limitations
          + under the License.
          +*/
          +
          +var path = require('path'),
          + fs = require('fs'),
          + child_process = require('child_process'),
          + Q = require('q'),
          + events = require('../events'),
          + os = require('os'),
          + isWindows = (os.platform().substr(0,3) === 'win');
          +
          +/**
          + * Implements functionality to run plugin level hooks.
          + * Hooks are defined in plugin config file as <script> elements.
          + */
          +module.exports = {
          + /**
          + * Fires specific plugin hook: 'preinstall', 'afterinstall', 'uninstall', etc.
          + * Async. Returns promise.
          + */
          + fire : function(type, plugin_id, pluginElement, platform, project_dir, plugin_dir) {
          + // args check
          + if (!type)

          { + throw Error('hook type is not specified'); + }

          +
          + events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.');
          +
          + var scriptTypes = getScriptTypesForHook(type);
          + if (scriptTypes == null) {
          — End diff –

          `getScriptTypesForHook` actually returns `undefined` when there is no matching type. Why not just `(!scriptTypes)`

          githubbot ASF GitHub Bot added a comment - Github user ligaz commented on a diff in the pull request: https://github.com/apache/cordova-plugman/pull/74#discussion_r12255211 — Diff: src/util/hooks.js — @@ -0,0 +1,148 @@ +/** + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. +*/ + +var path = require('path'), + fs = require('fs'), + child_process = require('child_process'), + Q = require('q'), + events = require('../events'), + os = require('os'), + isWindows = (os.platform().substr(0,3) === 'win'); + +/** + * Implements functionality to run plugin level hooks. + * Hooks are defined in plugin config file as <script> elements. + */ +module.exports = { + /** + * Fires specific plugin hook: 'preinstall', 'afterinstall', 'uninstall', etc. + * Async. Returns promise. + */ + fire : function(type, plugin_id, pluginElement, platform, project_dir, plugin_dir) { + // args check + if (!type) { + throw Error('hook type is not specified'); + } + + events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.'); + + var scriptTypes = getScriptTypesForHook(type); + if (scriptTypes == null) { — End diff – `getScriptTypesForHook` actually returns `undefined` when there is no matching type. Why not just `(!scriptTypes)`
          githubbot ASF GitHub Bot added a comment -

          Github user ligaz commented on a diff in the pull request:

          https://github.com/apache/cordova-plugman/pull/74#discussion_r12255215

          — Diff: src/util/hooks.js —
          @@ -0,0 +1,148 @@
          +/**
          + Licensed to the Apache Software Foundation (ASF) under one
          + or more contributor license agreements. See the NOTICE file
          + distributed with this work for additional information
          + regarding copyright ownership. The ASF licenses this file
          + to you under the Apache License, Version 2.0 (the
          + "License"); you may not use this file except in compliance
          + with the License. You may obtain a copy of the License at
          +
          + http://www.apache.org/licenses/LICENSE-2.0
          +
          + Unless required by applicable law or agreed to in writing,
          + software distributed under the License is distributed on an
          + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
          + KIND, either express or implied. See the License for the
          + specific language governing permissions and limitations
          + under the License.
          +*/
          +
          +var path = require('path'),
          + fs = require('fs'),
          + child_process = require('child_process'),
          + Q = require('q'),
          + events = require('../events'),
          + os = require('os'),
          + isWindows = (os.platform().substr(0,3) === 'win');
          +
          +/**
          + * Implements functionality to run plugin level hooks.
          + * Hooks are defined in plugin config file as <script> elements.
          + */
          +module.exports = {
          + /**
          + * Fires specific plugin hook: 'preinstall', 'afterinstall', 'uninstall', etc.
          + * Async. Returns promise.
          + */
          + fire : function(type, plugin_id, pluginElement, platform, project_dir, plugin_dir) {
          + // args check
          + if (!type)

          { + throw Error('hook type is not specified'); + }

          +
          + events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.');
          +
          + var scriptTypes = getScriptTypesForHook(type);
          + if (scriptTypes == null)

          { + throw Error('unknown plugin hook type: "' + type + '"' ); + }

          +
          + var scriptFilesToRun = getScriptFiles(pluginElement, scriptTypes, platform);
          + var context =

          { + platform: platform, + projectDir: project_dir, + pluginDir: plugin_dir, + cmdLine: process.argv.join(' '), + pluginId: plugin_id + }

          ;
          +
          + return runScripts(scriptFilesToRun, context);
          + }
          +};
          +
          +/**
          + * Returns all script types represented corresponding hook event.
          + * Allows to use multiple script types for the same hook event (usage simplicity),
          + * For example:
          + * <script type='install' .. /> or <script type='postinstall' .. /> could be used
          + * to define 'afterinstall' hook.
          + */
          +function getScriptTypesForHook(hookType) {
          + var knownTypes =

          { + beforeinstall: ['beforeinstall', 'preinstall'], + afterinstall: ['install', 'afterinstall', 'postinstall'], + uninstall: ['uninstall'] + }

          +
          + return knownTypes[hookType.toLowerCase()];
          +}
          +
          +/**
          + * Gets all scripts from the plugin xml with the specified types.
          + */
          +function getScriptFiles(pluginElement, scriptTypes, platform) {
          + var scriptFiles= [];
          + var scriptElements = pluginElement.findall('./script').concat(
          + pluginElement.findall('./platform[@name="' + platform + '"]/script'));
          +
          + var pendingScripts = [];
          — End diff –

          Is this used?

          githubbot ASF GitHub Bot added a comment - Github user ligaz commented on a diff in the pull request: https://github.com/apache/cordova-plugman/pull/74#discussion_r12255215 — Diff: src/util/hooks.js — @@ -0,0 +1,148 @@ +/** + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. +*/ + +var path = require('path'), + fs = require('fs'), + child_process = require('child_process'), + Q = require('q'), + events = require('../events'), + os = require('os'), + isWindows = (os.platform().substr(0,3) === 'win'); + +/** + * Implements functionality to run plugin level hooks. + * Hooks are defined in plugin config file as <script> elements. + */ +module.exports = { + /** + * Fires specific plugin hook: 'preinstall', 'afterinstall', 'uninstall', etc. + * Async. Returns promise. + */ + fire : function(type, plugin_id, pluginElement, platform, project_dir, plugin_dir) { + // args check + if (!type) { + throw Error('hook type is not specified'); + } + + events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.'); + + var scriptTypes = getScriptTypesForHook(type); + if (scriptTypes == null) { + throw Error('unknown plugin hook type: "' + type + '"' ); + } + + var scriptFilesToRun = getScriptFiles(pluginElement, scriptTypes, platform); + var context = { + platform: platform, + projectDir: project_dir, + pluginDir: plugin_dir, + cmdLine: process.argv.join(' '), + pluginId: plugin_id + } ; + + return runScripts(scriptFilesToRun, context); + } +}; + +/** + * Returns all script types represented corresponding hook event. + * Allows to use multiple script types for the same hook event (usage simplicity), + * For example: + * <script type='install' .. /> or <script type='postinstall' .. /> could be used + * to define 'afterinstall' hook. + */ +function getScriptTypesForHook(hookType) { + var knownTypes = { + beforeinstall: ['beforeinstall', 'preinstall'], + afterinstall: ['install', 'afterinstall', 'postinstall'], + uninstall: ['uninstall'] + } + + return knownTypes [hookType.toLowerCase()] ; +} + +/** + * Gets all scripts from the plugin xml with the specified types. + */ +function getScriptFiles(pluginElement, scriptTypes, platform) { + var scriptFiles= []; + var scriptElements = pluginElement.findall('./script').concat( + pluginElement.findall('./platform [@name="' + platform + '"] /script')); + + var pendingScripts = []; — End diff – Is this used?
          githubbot ASF GitHub Bot added a comment -

          Github user ligaz commented on a diff in the pull request:

          https://github.com/apache/cordova-plugman/pull/74#discussion_r12255221

          — Diff: src/util/hooks.js —
          @@ -0,0 +1,148 @@
          +/**
          + Licensed to the Apache Software Foundation (ASF) under one
          + or more contributor license agreements. See the NOTICE file
          + distributed with this work for additional information
          + regarding copyright ownership. The ASF licenses this file
          + to you under the Apache License, Version 2.0 (the
          + "License"); you may not use this file except in compliance
          + with the License. You may obtain a copy of the License at
          +
          + http://www.apache.org/licenses/LICENSE-2.0
          +
          + Unless required by applicable law or agreed to in writing,
          + software distributed under the License is distributed on an
          + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
          + KIND, either express or implied. See the License for the
          + specific language governing permissions and limitations
          + under the License.
          +*/
          +
          +var path = require('path'),
          + fs = require('fs'),
          + child_process = require('child_process'),
          + Q = require('q'),
          + events = require('../events'),
          + os = require('os'),
          + isWindows = (os.platform().substr(0,3) === 'win');
          +
          +/**
          + * Implements functionality to run plugin level hooks.
          + * Hooks are defined in plugin config file as <script> elements.
          + */
          +module.exports = {
          + /**
          + * Fires specific plugin hook: 'preinstall', 'afterinstall', 'uninstall', etc.
          + * Async. Returns promise.
          + */
          + fire : function(type, plugin_id, pluginElement, platform, project_dir, plugin_dir) {
          + // args check
          + if (!type)

          { + throw Error('hook type is not specified'); + }

          +
          + events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.');
          +
          + var scriptTypes = getScriptTypesForHook(type);
          + if (scriptTypes == null)

          { + throw Error('unknown plugin hook type: "' + type + '"' ); + }

          +
          + var scriptFilesToRun = getScriptFiles(pluginElement, scriptTypes, platform);
          + var context =

          { + platform: platform, + projectDir: project_dir, + pluginDir: plugin_dir, + cmdLine: process.argv.join(' '), + pluginId: plugin_id + }

          ;
          +
          + return runScripts(scriptFilesToRun, context);
          + }
          +};
          +
          +/**
          + * Returns all script types represented corresponding hook event.
          + * Allows to use multiple script types for the same hook event (usage simplicity),
          + * For example:
          + * <script type='install' .. /> or <script type='postinstall' .. /> could be used
          + * to define 'afterinstall' hook.
          + */
          +function getScriptTypesForHook(hookType) {
          + var knownTypes =

          { + beforeinstall: ['beforeinstall', 'preinstall'], + afterinstall: ['install', 'afterinstall', 'postinstall'], + uninstall: ['uninstall'] + }

          +
          + return knownTypes[hookType.toLowerCase()];
          +}
          +
          +/**
          + * Gets all scripts from the plugin xml with the specified types.
          + */
          +function getScriptFiles(pluginElement, scriptTypes, platform) {
          + var scriptFiles= [];
          + var scriptElements = pluginElement.findall('./script').concat(
          + pluginElement.findall('./platform[@name="' + platform + '"]/script'));
          +
          + var pendingScripts = [];
          +
          + scriptElements.forEach(function(script){
          — End diff –

          Why not use [`Array.prototype.map`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/map) here?

          githubbot ASF GitHub Bot added a comment - Github user ligaz commented on a diff in the pull request: https://github.com/apache/cordova-plugman/pull/74#discussion_r12255221 — Diff: src/util/hooks.js — @@ -0,0 +1,148 @@ +/** + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. +*/ + +var path = require('path'), + fs = require('fs'), + child_process = require('child_process'), + Q = require('q'), + events = require('../events'), + os = require('os'), + isWindows = (os.platform().substr(0,3) === 'win'); + +/** + * Implements functionality to run plugin level hooks. + * Hooks are defined in plugin config file as <script> elements. + */ +module.exports = { + /** + * Fires specific plugin hook: 'preinstall', 'afterinstall', 'uninstall', etc. + * Async. Returns promise. + */ + fire : function(type, plugin_id, pluginElement, platform, project_dir, plugin_dir) { + // args check + if (!type) { + throw Error('hook type is not specified'); + } + + events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.'); + + var scriptTypes = getScriptTypesForHook(type); + if (scriptTypes == null) { + throw Error('unknown plugin hook type: "' + type + '"' ); + } + + var scriptFilesToRun = getScriptFiles(pluginElement, scriptTypes, platform); + var context = { + platform: platform, + projectDir: project_dir, + pluginDir: plugin_dir, + cmdLine: process.argv.join(' '), + pluginId: plugin_id + } ; + + return runScripts(scriptFilesToRun, context); + } +}; + +/** + * Returns all script types represented corresponding hook event. + * Allows to use multiple script types for the same hook event (usage simplicity), + * For example: + * <script type='install' .. /> or <script type='postinstall' .. /> could be used + * to define 'afterinstall' hook. + */ +function getScriptTypesForHook(hookType) { + var knownTypes = { + beforeinstall: ['beforeinstall', 'preinstall'], + afterinstall: ['install', 'afterinstall', 'postinstall'], + uninstall: ['uninstall'] + } + + return knownTypes [hookType.toLowerCase()] ; +} + +/** + * Gets all scripts from the plugin xml with the specified types. + */ +function getScriptFiles(pluginElement, scriptTypes, platform) { + var scriptFiles= []; + var scriptElements = pluginElement.findall('./script').concat( + pluginElement.findall('./platform [@name="' + platform + '"] /script')); + + var pendingScripts = []; + + scriptElements.forEach(function(script){ — End diff – Why not use [`Array.prototype.map`] ( https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/map ) here?
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on a diff in the pull request:

          https://github.com/apache/cordova-plugman/pull/74#discussion_r12257679

          — Diff: src/util/hooks.js —
          @@ -0,0 +1,148 @@
          +/**
          + Licensed to the Apache Software Foundation (ASF) under one
          + or more contributor license agreements. See the NOTICE file
          + distributed with this work for additional information
          + regarding copyright ownership. The ASF licenses this file
          + to you under the Apache License, Version 2.0 (the
          + "License"); you may not use this file except in compliance
          + with the License. You may obtain a copy of the License at
          +
          + http://www.apache.org/licenses/LICENSE-2.0
          +
          + Unless required by applicable law or agreed to in writing,
          + software distributed under the License is distributed on an
          + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
          + KIND, either express or implied. See the License for the
          + specific language governing permissions and limitations
          + under the License.
          +*/
          +
          +var path = require('path'),
          + fs = require('fs'),
          + child_process = require('child_process'),
          + Q = require('q'),
          + events = require('../events'),
          + os = require('os'),
          + isWindows = (os.platform().substr(0,3) === 'win');
          +
          +/**
          + * Implements functionality to run plugin level hooks.
          + * Hooks are defined in plugin config file as <script> elements.
          + */
          +module.exports = {
          + /**
          + * Fires specific plugin hook: 'preinstall', 'afterinstall', 'uninstall', etc.
          + * Async. Returns promise.
          + */
          + fire : function(type, plugin_id, pluginElement, platform, project_dir, plugin_dir) {
          + // args check
          + if (!type)

          { + throw Error('hook type is not specified'); + }

          +
          + events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.');
          +
          + var scriptTypes = getScriptTypesForHook(type);
          + if (scriptTypes == null)

          { + throw Error('unknown plugin hook type: "' + type + '"' ); + }

          +
          + var scriptFilesToRun = getScriptFiles(pluginElement, scriptTypes, platform);
          + var context =

          { + platform: platform, + projectDir: project_dir, + pluginDir: plugin_dir, + cmdLine: process.argv.join(' '), + pluginId: plugin_id + }

          ;
          +
          + return runScripts(scriptFilesToRun, context);
          + }
          +};
          +
          +/**
          + * Returns all script types represented corresponding hook event.
          + * Allows to use multiple script types for the same hook event (usage simplicity),
          + * For example:
          + * <script type='install' .. /> or <script type='postinstall' .. /> could be used
          + * to define 'afterinstall' hook.
          + */
          +function getScriptTypesForHook(hookType) {
          + var knownTypes =

          { + beforeinstall: ['beforeinstall', 'preinstall'], + afterinstall: ['install', 'afterinstall', 'postinstall'], + uninstall: ['uninstall'] + }

          +
          + return knownTypes[hookType.toLowerCase()];
          +}
          +
          +/**
          + * Gets all scripts from the plugin xml with the specified types.
          + */
          +function getScriptFiles(pluginElement, scriptTypes, platform) {
          + var scriptFiles= [];
          + var scriptElements = pluginElement.findall('./script').concat(
          + pluginElement.findall('./platform[@name="' + platform + '"]/script'));
          +
          + var pendingScripts = [];
          +
          + scriptElements.forEach(function(script){
          — End diff –

          Actually no, we are interested only in scripts with specified type.

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on a diff in the pull request: https://github.com/apache/cordova-plugman/pull/74#discussion_r12257679 — Diff: src/util/hooks.js — @@ -0,0 +1,148 @@ +/** + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. +*/ + +var path = require('path'), + fs = require('fs'), + child_process = require('child_process'), + Q = require('q'), + events = require('../events'), + os = require('os'), + isWindows = (os.platform().substr(0,3) === 'win'); + +/** + * Implements functionality to run plugin level hooks. + * Hooks are defined in plugin config file as <script> elements. + */ +module.exports = { + /** + * Fires specific plugin hook: 'preinstall', 'afterinstall', 'uninstall', etc. + * Async. Returns promise. + */ + fire : function(type, plugin_id, pluginElement, platform, project_dir, plugin_dir) { + // args check + if (!type) { + throw Error('hook type is not specified'); + } + + events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.'); + + var scriptTypes = getScriptTypesForHook(type); + if (scriptTypes == null) { + throw Error('unknown plugin hook type: "' + type + '"' ); + } + + var scriptFilesToRun = getScriptFiles(pluginElement, scriptTypes, platform); + var context = { + platform: platform, + projectDir: project_dir, + pluginDir: plugin_dir, + cmdLine: process.argv.join(' '), + pluginId: plugin_id + } ; + + return runScripts(scriptFilesToRun, context); + } +}; + +/** + * Returns all script types represented corresponding hook event. + * Allows to use multiple script types for the same hook event (usage simplicity), + * For example: + * <script type='install' .. /> or <script type='postinstall' .. /> could be used + * to define 'afterinstall' hook. + */ +function getScriptTypesForHook(hookType) { + var knownTypes = { + beforeinstall: ['beforeinstall', 'preinstall'], + afterinstall: ['install', 'afterinstall', 'postinstall'], + uninstall: ['uninstall'] + } + + return knownTypes [hookType.toLowerCase()] ; +} + +/** + * Gets all scripts from the plugin xml with the specified types. + */ +function getScriptFiles(pluginElement, scriptTypes, platform) { + var scriptFiles= []; + var scriptElements = pluginElement.findall('./script').concat( + pluginElement.findall('./platform [@name="' + platform + '"] /script')); + + var pendingScripts = []; + + scriptElements.forEach(function(script){ — End diff – Actually no, we are interested only in scripts with specified type.
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on a diff in the pull request:

          https://github.com/apache/cordova-plugman/pull/74#discussion_r12257681

          — Diff: src/util/hooks.js —
          @@ -0,0 +1,148 @@
          +/**
          + Licensed to the Apache Software Foundation (ASF) under one
          + or more contributor license agreements. See the NOTICE file
          + distributed with this work for additional information
          + regarding copyright ownership. The ASF licenses this file
          + to you under the Apache License, Version 2.0 (the
          + "License"); you may not use this file except in compliance
          + with the License. You may obtain a copy of the License at
          +
          + http://www.apache.org/licenses/LICENSE-2.0
          +
          + Unless required by applicable law or agreed to in writing,
          + software distributed under the License is distributed on an
          + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
          + KIND, either express or implied. See the License for the
          + specific language governing permissions and limitations
          + under the License.
          +*/
          +
          +var path = require('path'),
          + fs = require('fs'),
          + child_process = require('child_process'),
          + Q = require('q'),
          + events = require('../events'),
          + os = require('os'),
          + isWindows = (os.platform().substr(0,3) === 'win');
          +
          +/**
          + * Implements functionality to run plugin level hooks.
          + * Hooks are defined in plugin config file as <script> elements.
          + */
          +module.exports = {
          + /**
          + * Fires specific plugin hook: 'preinstall', 'afterinstall', 'uninstall', etc.
          + * Async. Returns promise.
          + */
          + fire : function(type, plugin_id, pluginElement, platform, project_dir, plugin_dir) {
          + // args check
          + if (!type)

          { + throw Error('hook type is not specified'); + }

          +
          + events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.');
          +
          + var scriptTypes = getScriptTypesForHook(type);
          + if (scriptTypes == null)

          { + throw Error('unknown plugin hook type: "' + type + '"' ); + }

          +
          + var scriptFilesToRun = getScriptFiles(pluginElement, scriptTypes, platform);
          + var context =

          { + platform: platform, + projectDir: project_dir, + pluginDir: plugin_dir, + cmdLine: process.argv.join(' '), + pluginId: plugin_id + }

          ;
          +
          + return runScripts(scriptFilesToRun, context);
          + }
          +};
          +
          +/**
          + * Returns all script types represented corresponding hook event.
          + * Allows to use multiple script types for the same hook event (usage simplicity),
          + * For example:
          + * <script type='install' .. /> or <script type='postinstall' .. /> could be used
          + * to define 'afterinstall' hook.
          + */
          +function getScriptTypesForHook(hookType) {
          + var knownTypes =

          { + beforeinstall: ['beforeinstall', 'preinstall'], + afterinstall: ['install', 'afterinstall', 'postinstall'], + uninstall: ['uninstall'] + }

          +
          + return knownTypes[hookType.toLowerCase()];
          +}
          +
          +/**
          + * Gets all scripts from the plugin xml with the specified types.
          + */
          +function getScriptFiles(pluginElement, scriptTypes, platform) {
          + var scriptFiles= [];
          + var scriptElements = pluginElement.findall('./script').concat(
          + pluginElement.findall('./platform[@name="' + platform + '"]/script'));
          +
          + var pendingScripts = [];
          — End diff –

          Agree, will remove

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on a diff in the pull request: https://github.com/apache/cordova-plugman/pull/74#discussion_r12257681 — Diff: src/util/hooks.js — @@ -0,0 +1,148 @@ +/** + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. +*/ + +var path = require('path'), + fs = require('fs'), + child_process = require('child_process'), + Q = require('q'), + events = require('../events'), + os = require('os'), + isWindows = (os.platform().substr(0,3) === 'win'); + +/** + * Implements functionality to run plugin level hooks. + * Hooks are defined in plugin config file as <script> elements. + */ +module.exports = { + /** + * Fires specific plugin hook: 'preinstall', 'afterinstall', 'uninstall', etc. + * Async. Returns promise. + */ + fire : function(type, plugin_id, pluginElement, platform, project_dir, plugin_dir) { + // args check + if (!type) { + throw Error('hook type is not specified'); + } + + events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.'); + + var scriptTypes = getScriptTypesForHook(type); + if (scriptTypes == null) { + throw Error('unknown plugin hook type: "' + type + '"' ); + } + + var scriptFilesToRun = getScriptFiles(pluginElement, scriptTypes, platform); + var context = { + platform: platform, + projectDir: project_dir, + pluginDir: plugin_dir, + cmdLine: process.argv.join(' '), + pluginId: plugin_id + } ; + + return runScripts(scriptFilesToRun, context); + } +}; + +/** + * Returns all script types represented corresponding hook event. + * Allows to use multiple script types for the same hook event (usage simplicity), + * For example: + * <script type='install' .. /> or <script type='postinstall' .. /> could be used + * to define 'afterinstall' hook. + */ +function getScriptTypesForHook(hookType) { + var knownTypes = { + beforeinstall: ['beforeinstall', 'preinstall'], + afterinstall: ['install', 'afterinstall', 'postinstall'], + uninstall: ['uninstall'] + } + + return knownTypes [hookType.toLowerCase()] ; +} + +/** + * Gets all scripts from the plugin xml with the specified types. + */ +function getScriptFiles(pluginElement, scriptTypes, platform) { + var scriptFiles= []; + var scriptElements = pluginElement.findall('./script').concat( + pluginElement.findall('./platform [@name="' + platform + '"] /script')); + + var pendingScripts = []; — End diff – Agree, will remove
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on a diff in the pull request:

          https://github.com/apache/cordova-plugman/pull/74#discussion_r12257686

          — Diff: src/util/hooks.js —
          @@ -0,0 +1,148 @@
          +/**
          + Licensed to the Apache Software Foundation (ASF) under one
          + or more contributor license agreements. See the NOTICE file
          + distributed with this work for additional information
          + regarding copyright ownership. The ASF licenses this file
          + to you under the Apache License, Version 2.0 (the
          + "License"); you may not use this file except in compliance
          + with the License. You may obtain a copy of the License at
          +
          + http://www.apache.org/licenses/LICENSE-2.0
          +
          + Unless required by applicable law or agreed to in writing,
          + software distributed under the License is distributed on an
          + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
          + KIND, either express or implied. See the License for the
          + specific language governing permissions and limitations
          + under the License.
          +*/
          +
          +var path = require('path'),
          + fs = require('fs'),
          + child_process = require('child_process'),
          + Q = require('q'),
          + events = require('../events'),
          + os = require('os'),
          + isWindows = (os.platform().substr(0,3) === 'win');
          +
          +/**
          + * Implements functionality to run plugin level hooks.
          + * Hooks are defined in plugin config file as <script> elements.
          + */
          +module.exports = {
          + /**
          + * Fires specific plugin hook: 'preinstall', 'afterinstall', 'uninstall', etc.
          + * Async. Returns promise.
          + */
          + fire : function(type, plugin_id, pluginElement, platform, project_dir, plugin_dir) {
          + // args check
          + if (!type)

          { + throw Error('hook type is not specified'); + }

          +
          + events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.');
          +
          + var scriptTypes = getScriptTypesForHook(type);
          + if (scriptTypes == null) {
          — End diff –

          Agree (!scriptTypes) is better here.
          Btw, 'undefined == null' is true so it works correct

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on a diff in the pull request: https://github.com/apache/cordova-plugman/pull/74#discussion_r12257686 — Diff: src/util/hooks.js — @@ -0,0 +1,148 @@ +/** + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. +*/ + +var path = require('path'), + fs = require('fs'), + child_process = require('child_process'), + Q = require('q'), + events = require('../events'), + os = require('os'), + isWindows = (os.platform().substr(0,3) === 'win'); + +/** + * Implements functionality to run plugin level hooks. + * Hooks are defined in plugin config file as <script> elements. + */ +module.exports = { + /** + * Fires specific plugin hook: 'preinstall', 'afterinstall', 'uninstall', etc. + * Async. Returns promise. + */ + fire : function(type, plugin_id, pluginElement, platform, project_dir, plugin_dir) { + // args check + if (!type) { + throw Error('hook type is not specified'); + } + + events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.'); + + var scriptTypes = getScriptTypesForHook(type); + if (scriptTypes == null) { — End diff – Agree (!scriptTypes) is better here. Btw, 'undefined == null' is true so it works correct
          githubbot ASF GitHub Bot added a comment -

          Github user ligaz commented on a diff in the pull request:

          https://github.com/apache/cordova-plugman/pull/74#discussion_r12259722

          — Diff: src/util/hooks.js —
          @@ -0,0 +1,148 @@
          +/**
          + Licensed to the Apache Software Foundation (ASF) under one
          + or more contributor license agreements. See the NOTICE file
          + distributed with this work for additional information
          + regarding copyright ownership. The ASF licenses this file
          + to you under the Apache License, Version 2.0 (the
          + "License"); you may not use this file except in compliance
          + with the License. You may obtain a copy of the License at
          +
          + http://www.apache.org/licenses/LICENSE-2.0
          +
          + Unless required by applicable law or agreed to in writing,
          + software distributed under the License is distributed on an
          + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
          + KIND, either express or implied. See the License for the
          + specific language governing permissions and limitations
          + under the License.
          +*/
          +
          +var path = require('path'),
          + fs = require('fs'),
          + child_process = require('child_process'),
          + Q = require('q'),
          + events = require('../events'),
          + os = require('os'),
          + isWindows = (os.platform().substr(0,3) === 'win');
          +
          +/**
          + * Implements functionality to run plugin level hooks.
          + * Hooks are defined in plugin config file as <script> elements.
          + */
          +module.exports = {
          + /**
          + * Fires specific plugin hook: 'preinstall', 'afterinstall', 'uninstall', etc.
          + * Async. Returns promise.
          + */
          + fire : function(type, plugin_id, pluginElement, platform, project_dir, plugin_dir) {
          + // args check
          + if (!type)

          { + throw Error('hook type is not specified'); + }

          +
          + events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.');
          +
          + var scriptTypes = getScriptTypesForHook(type);
          + if (scriptTypes == null)

          { + throw Error('unknown plugin hook type: "' + type + '"' ); + }

          +
          + var scriptFilesToRun = getScriptFiles(pluginElement, scriptTypes, platform);
          + var context =

          { + platform: platform, + projectDir: project_dir, + pluginDir: plugin_dir, + cmdLine: process.argv.join(' '), + pluginId: plugin_id + }

          ;
          +
          + return runScripts(scriptFilesToRun, context);
          + }
          +};
          +
          +/**
          + * Returns all script types represented corresponding hook event.
          + * Allows to use multiple script types for the same hook event (usage simplicity),
          + * For example:
          + * <script type='install' .. /> or <script type='postinstall' .. /> could be used
          + * to define 'afterinstall' hook.
          + */
          +function getScriptTypesForHook(hookType) {
          + var knownTypes =

          { + beforeinstall: ['beforeinstall', 'preinstall'], + afterinstall: ['install', 'afterinstall', 'postinstall'], + uninstall: ['uninstall'] + }

          +
          + return knownTypes[hookType.toLowerCase()];
          +}
          +
          +/**
          + * Gets all scripts from the plugin xml with the specified types.
          + */
          +function getScriptFiles(pluginElement, scriptTypes, platform) {
          + var scriptFiles= [];
          + var scriptElements = pluginElement.findall('./script').concat(
          + pluginElement.findall('./platform[@name="' + platform + '"]/script'));
          +
          + var pendingScripts = [];
          +
          + scriptElements.forEach(function(script){
          — End diff –

          Indeed you should use `filter` and `map`.

          githubbot ASF GitHub Bot added a comment - Github user ligaz commented on a diff in the pull request: https://github.com/apache/cordova-plugman/pull/74#discussion_r12259722 — Diff: src/util/hooks.js — @@ -0,0 +1,148 @@ +/** + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. +*/ + +var path = require('path'), + fs = require('fs'), + child_process = require('child_process'), + Q = require('q'), + events = require('../events'), + os = require('os'), + isWindows = (os.platform().substr(0,3) === 'win'); + +/** + * Implements functionality to run plugin level hooks. + * Hooks are defined in plugin config file as <script> elements. + */ +module.exports = { + /** + * Fires specific plugin hook: 'preinstall', 'afterinstall', 'uninstall', etc. + * Async. Returns promise. + */ + fire : function(type, plugin_id, pluginElement, platform, project_dir, plugin_dir) { + // args check + if (!type) { + throw Error('hook type is not specified'); + } + + events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.'); + + var scriptTypes = getScriptTypesForHook(type); + if (scriptTypes == null) { + throw Error('unknown plugin hook type: "' + type + '"' ); + } + + var scriptFilesToRun = getScriptFiles(pluginElement, scriptTypes, platform); + var context = { + platform: platform, + projectDir: project_dir, + pluginDir: plugin_dir, + cmdLine: process.argv.join(' '), + pluginId: plugin_id + } ; + + return runScripts(scriptFilesToRun, context); + } +}; + +/** + * Returns all script types represented corresponding hook event. + * Allows to use multiple script types for the same hook event (usage simplicity), + * For example: + * <script type='install' .. /> or <script type='postinstall' .. /> could be used + * to define 'afterinstall' hook. + */ +function getScriptTypesForHook(hookType) { + var knownTypes = { + beforeinstall: ['beforeinstall', 'preinstall'], + afterinstall: ['install', 'afterinstall', 'postinstall'], + uninstall: ['uninstall'] + } + + return knownTypes [hookType.toLowerCase()] ; +} + +/** + * Gets all scripts from the plugin xml with the specified types. + */ +function getScriptFiles(pluginElement, scriptTypes, platform) { + var scriptFiles= []; + var scriptElements = pluginElement.findall('./script').concat( + pluginElement.findall('./platform [@name="' + platform + '"] /script')); + + var pendingScripts = []; + + scriptElements.forEach(function(script){ — End diff – Indeed you should use `filter` and `map`.
          githubbot ASF GitHub Bot added a comment -

          Github user ligaz commented on the pull request:

          https://github.com/apache/cordova-plugman/pull/74#issuecomment-42123996

          Adding a couple of unit tests will be great :cop:

          githubbot ASF GitHub Bot added a comment - Github user ligaz commented on the pull request: https://github.com/apache/cordova-plugman/pull/74#issuecomment-42123996 Adding a couple of unit tests will be great :cop:
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on the pull request:

          https://github.com/apache/cordova-plugman/pull/74#issuecomment-42234483

          Totally agree, will do as soon as cordova-lib work is stabilized so I can move the code to appropriate repo and perform final tests. @ligaz, thx for review.

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on the pull request: https://github.com/apache/cordova-plugman/pull/74#issuecomment-42234483 Totally agree, will do as soon as cordova-lib work is stabilized so I can move the code to appropriate repo and perform final tests. @ligaz, thx for review.
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on the pull request:

          https://github.com/apache/cordova-plugman/pull/74#issuecomment-43280272

          Moved to appropriate repo, closing...
          https://github.com/apache/cordova-lib/pull/12

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on the pull request: https://github.com/apache/cordova-plugman/pull/74#issuecomment-43280272 Moved to appropriate repo, closing... https://github.com/apache/cordova-lib/pull/12
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov closed the pull request at:

          https://github.com/apache/cordova-plugman/pull/74

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov closed the pull request at: https://github.com/apache/cordova-plugman/pull/74
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on the pull request:

          https://github.com/apache/cordova-lib/pull/12#issuecomment-47623059

          New plugin hooks implementation is coming...closing this one

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on the pull request: https://github.com/apache/cordova-lib/pull/12#issuecomment-47623059 New plugin hooks implementation is coming...closing this one
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov closed the pull request at:

          https://github.com/apache/cordova-lib/pull/12

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov closed the pull request at: https://github.com/apache/cordova-lib/pull/12
          githubbot ASF GitHub Bot added a comment -

          GitHub user sgrebnov opened a pull request:

          https://github.com/apache/cordova-lib/pull/55

          CB-6481 Add unified hooks support for cordova app and plugins

          https://issues.apache.org/jira/browse/CB-6481

          Added the following changes and new features

          • Hooks can be defined in .cordova/hooks/hook_type, hooks/hook_type directories, *config.xml* (by application developers) and plugins/.../*plugin.xml* (by plugins developers)
          • Javascript hooks retrieved from config.xml and plugins/.../plugin.xml will be run via new module loader with special *Context argument* passed. This object represents current hook script execution context including hook type, cordova version, paths, plugin info and other special utility modules.
          • Introduced *before_plugin_install, **after_plugin_install* and *before_plugin_uninstall* hooks.

          See updated docs for more details:
          https://github.com/MSOpenTech/cordova-lib/commit/952690b7d7b42962b4e246c2b84c309846bf8750?short_path=8c918a9#diff-8c918a9f452feb5e3ef3339749806fc3

          Please note, that there are some remaining work items here below before we can merge it. The PR has been sent to get community feedback and adjust implementation while we are working on unit tests.

          • Write unit test (in-progress)
          • Replace original Hooker with new implementation everywhere, Currently 'before_build' is supported for general application hooks.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/MSOpenTech/cordova-lib CB-6481-hooks

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/cordova-lib/pull/55.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #55


          commit d51b5ac509014ccd28351cb5b853cef0a4188358
          Author: daserge <daserge@yandex.ru>
          Date: 2014-07-09T12:08:59Z

          CB-6481 Added unified hooks support for cordova app and plugins

          • Hooks can be defined in .cordova/hooks/hook_type, hooks/hook_type directories, config.xml and plugins/.../plugin.xml
          • Javascript hooks retrieved from config.xml and plugins/.../plugin.xml will be run via new module loader
          • Introduced before_plugin_install, after_plugin_install and before_plugin_uninstall hooks

          commit 952690b7d7b42962b4e246c2b84c309846bf8750
          Author: daserge <daserge@yandex.ru>
          Date: 2014-07-09T12:20:11Z

          CB-6481 Updated hooks documentation


          githubbot ASF GitHub Bot added a comment - GitHub user sgrebnov opened a pull request: https://github.com/apache/cordova-lib/pull/55 CB-6481 Add unified hooks support for cordova app and plugins https://issues.apache.org/jira/browse/CB-6481 Added the following changes and new features Hooks can be defined in .cordova/hooks/hook_type, hooks/hook_type directories, * config.xml * (by application developers) and plugins/.../* plugin.xml * (by plugins developers) Javascript hooks retrieved from config.xml and plugins/.../plugin.xml will be run via new module loader with special * Context argument * passed. This object represents current hook script execution context including hook type, cordova version, paths, plugin info and other special utility modules. Introduced * before_plugin_install , **after_plugin_install * and * before_plugin_uninstall * hooks. See updated docs for more details: https://github.com/MSOpenTech/cordova-lib/commit/952690b7d7b42962b4e246c2b84c309846bf8750?short_path=8c918a9#diff-8c918a9f452feb5e3ef3339749806fc3 Please note, that there are some remaining work items here below before we can merge it. The PR has been sent to get community feedback and adjust implementation while we are working on unit tests. Write unit test (in-progress) Replace original Hooker with new implementation everywhere, Currently 'before_build' is supported for general application hooks. You can merge this pull request into a Git repository by running: $ git pull https://github.com/MSOpenTech/cordova-lib CB-6481 -hooks Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cordova-lib/pull/55.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #55 commit d51b5ac509014ccd28351cb5b853cef0a4188358 Author: daserge <daserge@yandex.ru> Date: 2014-07-09T12:08:59Z CB-6481 Added unified hooks support for cordova app and plugins Hooks can be defined in .cordova/hooks/hook_type, hooks/hook_type directories, config.xml and plugins/.../plugin.xml Javascript hooks retrieved from config.xml and plugins/.../plugin.xml will be run via new module loader Introduced before_plugin_install, after_plugin_install and before_plugin_uninstall hooks commit 952690b7d7b42962b4e246c2b84c309846bf8750 Author: daserge <daserge@yandex.ru> Date: 2014-07-09T12:20:11Z CB-6481 Updated hooks documentation
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-48701880

          Addressed docs review notes: deprecated .cordova/hooks directory

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-48701880 Addressed docs review notes: deprecated .cordova/hooks directory
          githubbot ASF GitHub Bot added a comment -

          Github user kamrik commented on a diff in the pull request:

          https://github.com/apache/cordova-lib/pull/55#discussion_r14820487

          — Diff: cordova-lib/src/hooks/ScriptsFinder.js —
          @@ -0,0 +1,158 @@
          +/**
          + Licensed to the Apache Software Foundation (ASF) under one
          + or more contributor license agreements. See the NOTICE file
          + distributed with this work for additional information
          + regarding copyright ownership. The ASF licenses this file
          + to you under the Apache License, Version 2.0 (the
          + "License"); you may not use this file except in compliance
          + with the License. You may obtain a copy of the License at
          +
          + http://www.apache.org/licenses/LICENSE-2.0
          +
          + Unless required by applicable law or agreed to in writing,
          + software distributed under the License is distributed on an
          + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
          + KIND, either express or implied. See the License for the
          + specific language governing permissions and limitations
          + under the License.
          + */
          +
          +var path = require('path'),
          + fs = require('fs'),
          + cordovaUtil = require('../cordova/util'),
          + events = require('../events'),
          + Q = require('q'),
          + plugin = require('../cordova/plugin'),
          + PluginInfo = require('../PluginInfo'),
          + ConfigParser = require('../configparser/ConfigParser'),
          + CordovaError = require('../CordovaError'),
          + Context = require('./Context');
          +
          +/**
          + * Implements logic to retrieve hook script files defined in special folders and configuration
          + * files: config.xml, hooks/hook_type, plugins/../plugin.xml, etc
          + */
          +module.exports = {
          + /**
          + * Returns all script files for the hook type specified.
          + */
          + getHookScripts: function(hook, opts) {
          + // args check
          + if (!hook)

          { + throw new CordovaError('hook type is not specified'); + }

          + return getApplicationHookScripts(hook, opts)
          + .concat(getPluginsHookScripts(hook, opts));
          + }
          +};
          +
          +/**
          + * Returns script files defined on application level.
          + * They are stored in .cordova/hooks folders and in config.xml.
          + */
          +function getApplicationHookScripts(hook, opts) {
          + // args check
          + if (!hook)

          { + throw new CordovaError('hook type is not specified'); + }

          + return getApplicationHookScriptsFromDir(path.join(opts.projectRoot, '.cordova', 'hooks', hook))
          + .concat(getApplicationHookScriptsFromDir(path.join(opts.projectRoot, 'hooks', hook)))
          + .concat(getScriptsFromConfigXml(hook, opts));
          +}
          +
          +/**
          + * Returns script files defined by plugin developers as part of plugin.xml.
          + */
          +function getPluginsHookScripts(hook, opts) {
          + // args check
          + if (!hook) {
          + throw new CordovaError('hook type is not specified');
          — End diff –

          I think this should be Error rather than CordovaError. If we get here, it means something went very wrong, probably it's a but, not a case of bad user input or other problems we expect to happen occasionally.

          githubbot ASF GitHub Bot added a comment - Github user kamrik commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/55#discussion_r14820487 — Diff: cordova-lib/src/hooks/ScriptsFinder.js — @@ -0,0 +1,158 @@ +/** + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. + */ + +var path = require('path'), + fs = require('fs'), + cordovaUtil = require('../cordova/util'), + events = require('../events'), + Q = require('q'), + plugin = require('../cordova/plugin'), + PluginInfo = require('../PluginInfo'), + ConfigParser = require('../configparser/ConfigParser'), + CordovaError = require('../CordovaError'), + Context = require('./Context'); + +/** + * Implements logic to retrieve hook script files defined in special folders and configuration + * files: config.xml, hooks/hook_type, plugins/../plugin.xml, etc + */ +module.exports = { + /** + * Returns all script files for the hook type specified. + */ + getHookScripts: function(hook, opts) { + // args check + if (!hook) { + throw new CordovaError('hook type is not specified'); + } + return getApplicationHookScripts(hook, opts) + .concat(getPluginsHookScripts(hook, opts)); + } +}; + +/** + * Returns script files defined on application level. + * They are stored in .cordova/hooks folders and in config.xml. + */ +function getApplicationHookScripts(hook, opts) { + // args check + if (!hook) { + throw new CordovaError('hook type is not specified'); + } + return getApplicationHookScriptsFromDir(path.join(opts.projectRoot, '.cordova', 'hooks', hook)) + .concat(getApplicationHookScriptsFromDir(path.join(opts.projectRoot, 'hooks', hook))) + .concat(getScriptsFromConfigXml(hook, opts)); +} + +/** + * Returns script files defined by plugin developers as part of plugin.xml. + */ +function getPluginsHookScripts(hook, opts) { + // args check + if (!hook) { + throw new CordovaError('hook type is not specified'); — End diff – I think this should be Error rather than CordovaError. If we get here, it means something went very wrong, probably it's a but, not a case of bad user input or other problems we expect to happen occasionally.
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on a diff in the pull request:

          https://github.com/apache/cordova-lib/pull/55#discussion_r14821551

          — Diff: cordova-lib/src/hooks/ScriptsFinder.js —
          @@ -0,0 +1,158 @@
          +/**
          + Licensed to the Apache Software Foundation (ASF) under one
          + or more contributor license agreements. See the NOTICE file
          + distributed with this work for additional information
          + regarding copyright ownership. The ASF licenses this file
          + to you under the Apache License, Version 2.0 (the
          + "License"); you may not use this file except in compliance
          + with the License. You may obtain a copy of the License at
          +
          + http://www.apache.org/licenses/LICENSE-2.0
          +
          + Unless required by applicable law or agreed to in writing,
          + software distributed under the License is distributed on an
          + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
          + KIND, either express or implied. See the License for the
          + specific language governing permissions and limitations
          + under the License.
          + */
          +
          +var path = require('path'),
          + fs = require('fs'),
          + cordovaUtil = require('../cordova/util'),
          + events = require('../events'),
          + Q = require('q'),
          + plugin = require('../cordova/plugin'),
          + PluginInfo = require('../PluginInfo'),
          + ConfigParser = require('../configparser/ConfigParser'),
          + CordovaError = require('../CordovaError'),
          + Context = require('./Context');
          +
          +/**
          + * Implements logic to retrieve hook script files defined in special folders and configuration
          + * files: config.xml, hooks/hook_type, plugins/../plugin.xml, etc
          + */
          +module.exports = {
          + /**
          + * Returns all script files for the hook type specified.
          + */
          + getHookScripts: function(hook, opts) {
          + // args check
          + if (!hook)

          { + throw new CordovaError('hook type is not specified'); + }

          + return getApplicationHookScripts(hook, opts)
          + .concat(getPluginsHookScripts(hook, opts));
          + }
          +};
          +
          +/**
          + * Returns script files defined on application level.
          + * They are stored in .cordova/hooks folders and in config.xml.
          + */
          +function getApplicationHookScripts(hook, opts) {
          + // args check
          + if (!hook)

          { + throw new CordovaError('hook type is not specified'); + }

          + return getApplicationHookScriptsFromDir(path.join(opts.projectRoot, '.cordova', 'hooks', hook))
          + .concat(getApplicationHookScriptsFromDir(path.join(opts.projectRoot, 'hooks', hook)))
          + .concat(getScriptsFromConfigXml(hook, opts));
          +}
          +
          +/**
          + * Returns script files defined by plugin developers as part of plugin.xml.
          + */
          +function getPluginsHookScripts(hook, opts) {
          + // args check
          + if (!hook) {
          + throw new CordovaError('hook type is not specified');
          — End diff –

          Thx @kamrik - I'll review this. Btw, is there any special guideline where we should use Error vs CordovaError?

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/55#discussion_r14821551 — Diff: cordova-lib/src/hooks/ScriptsFinder.js — @@ -0,0 +1,158 @@ +/** + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. + */ + +var path = require('path'), + fs = require('fs'), + cordovaUtil = require('../cordova/util'), + events = require('../events'), + Q = require('q'), + plugin = require('../cordova/plugin'), + PluginInfo = require('../PluginInfo'), + ConfigParser = require('../configparser/ConfigParser'), + CordovaError = require('../CordovaError'), + Context = require('./Context'); + +/** + * Implements logic to retrieve hook script files defined in special folders and configuration + * files: config.xml, hooks/hook_type, plugins/../plugin.xml, etc + */ +module.exports = { + /** + * Returns all script files for the hook type specified. + */ + getHookScripts: function(hook, opts) { + // args check + if (!hook) { + throw new CordovaError('hook type is not specified'); + } + return getApplicationHookScripts(hook, opts) + .concat(getPluginsHookScripts(hook, opts)); + } +}; + +/** + * Returns script files defined on application level. + * They are stored in .cordova/hooks folders and in config.xml. + */ +function getApplicationHookScripts(hook, opts) { + // args check + if (!hook) { + throw new CordovaError('hook type is not specified'); + } + return getApplicationHookScriptsFromDir(path.join(opts.projectRoot, '.cordova', 'hooks', hook)) + .concat(getApplicationHookScriptsFromDir(path.join(opts.projectRoot, 'hooks', hook))) + .concat(getScriptsFromConfigXml(hook, opts)); +} + +/** + * Returns script files defined by plugin developers as part of plugin.xml. + */ +function getPluginsHookScripts(hook, opts) { + // args check + if (!hook) { + throw new CordovaError('hook type is not specified'); — End diff – Thx @kamrik - I'll review this. Btw, is there any special guideline where we should use Error vs CordovaError?
          githubbot ASF GitHub Bot added a comment -

          Github user kamrik commented on a diff in the pull request:

          https://github.com/apache/cordova-lib/pull/55#discussion_r14821829

          — Diff: cordova-lib/src/hooks/ScriptsFinder.js —
          @@ -0,0 +1,158 @@
          +/**
          + Licensed to the Apache Software Foundation (ASF) under one
          + or more contributor license agreements. See the NOTICE file
          + distributed with this work for additional information
          + regarding copyright ownership. The ASF licenses this file
          + to you under the Apache License, Version 2.0 (the
          + "License"); you may not use this file except in compliance
          + with the License. You may obtain a copy of the License at
          +
          + http://www.apache.org/licenses/LICENSE-2.0
          +
          + Unless required by applicable law or agreed to in writing,
          + software distributed under the License is distributed on an
          + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
          + KIND, either express or implied. See the License for the
          + specific language governing permissions and limitations
          + under the License.
          + */
          +
          +var path = require('path'),
          + fs = require('fs'),
          + cordovaUtil = require('../cordova/util'),
          + events = require('../events'),
          + Q = require('q'),
          + plugin = require('../cordova/plugin'),
          + PluginInfo = require('../PluginInfo'),
          + ConfigParser = require('../configparser/ConfigParser'),
          + CordovaError = require('../CordovaError'),
          + Context = require('./Context');
          +
          +/**
          + * Implements logic to retrieve hook script files defined in special folders and configuration
          + * files: config.xml, hooks/hook_type, plugins/../plugin.xml, etc
          + */
          +module.exports = {
          + /**
          + * Returns all script files for the hook type specified.
          + */
          + getHookScripts: function(hook, opts) {
          + // args check
          + if (!hook)

          { + throw new CordovaError('hook type is not specified'); + }

          + return getApplicationHookScripts(hook, opts)
          + .concat(getPluginsHookScripts(hook, opts));
          + }
          +};
          +
          +/**
          + * Returns script files defined on application level.
          + * They are stored in .cordova/hooks folders and in config.xml.
          + */
          +function getApplicationHookScripts(hook, opts) {
          + // args check
          + if (!hook)

          { + throw new CordovaError('hook type is not specified'); + }

          + return getApplicationHookScriptsFromDir(path.join(opts.projectRoot, '.cordova', 'hooks', hook))
          + .concat(getApplicationHookScriptsFromDir(path.join(opts.projectRoot, 'hooks', hook)))
          + .concat(getScriptsFromConfigXml(hook, opts));
          +}
          +
          +/**
          + * Returns script files defined by plugin developers as part of plugin.xml.
          + */
          +function getPluginsHookScripts(hook, opts) {
          + // args check
          + if (!hook) {
          + throw new CordovaError('hook type is not specified');
          — End diff –

          Not really a guideline but: CordovaError is only printed as the message while Error results in a full stack trace. So I would say, use CordovaError for errors that you expect to happen due to bad user input. If it's happening because of bad code that we wrote, it should be an Error().

          githubbot ASF GitHub Bot added a comment - Github user kamrik commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/55#discussion_r14821829 — Diff: cordova-lib/src/hooks/ScriptsFinder.js — @@ -0,0 +1,158 @@ +/** + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. + */ + +var path = require('path'), + fs = require('fs'), + cordovaUtil = require('../cordova/util'), + events = require('../events'), + Q = require('q'), + plugin = require('../cordova/plugin'), + PluginInfo = require('../PluginInfo'), + ConfigParser = require('../configparser/ConfigParser'), + CordovaError = require('../CordovaError'), + Context = require('./Context'); + +/** + * Implements logic to retrieve hook script files defined in special folders and configuration + * files: config.xml, hooks/hook_type, plugins/../plugin.xml, etc + */ +module.exports = { + /** + * Returns all script files for the hook type specified. + */ + getHookScripts: function(hook, opts) { + // args check + if (!hook) { + throw new CordovaError('hook type is not specified'); + } + return getApplicationHookScripts(hook, opts) + .concat(getPluginsHookScripts(hook, opts)); + } +}; + +/** + * Returns script files defined on application level. + * They are stored in .cordova/hooks folders and in config.xml. + */ +function getApplicationHookScripts(hook, opts) { + // args check + if (!hook) { + throw new CordovaError('hook type is not specified'); + } + return getApplicationHookScriptsFromDir(path.join(opts.projectRoot, '.cordova', 'hooks', hook)) + .concat(getApplicationHookScriptsFromDir(path.join(opts.projectRoot, 'hooks', hook))) + .concat(getScriptsFromConfigXml(hook, opts)); +} + +/** + * Returns script files defined by plugin developers as part of plugin.xml. + */ +function getPluginsHookScripts(hook, opts) { + // args check + if (!hook) { + throw new CordovaError('hook type is not specified'); — End diff – Not really a guideline but: CordovaError is only printed as the message while Error results in a full stack trace. So I would say, use CordovaError for errors that you expect to happen due to bad user input. If it's happening because of bad code that we wrote, it should be an Error().
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on a diff in the pull request:

          https://github.com/apache/cordova-lib/pull/55#discussion_r14822338

          — Diff: cordova-lib/src/hooks/ScriptsFinder.js —
          @@ -0,0 +1,158 @@
          +/**
          + Licensed to the Apache Software Foundation (ASF) under one
          + or more contributor license agreements. See the NOTICE file
          + distributed with this work for additional information
          + regarding copyright ownership. The ASF licenses this file
          + to you under the Apache License, Version 2.0 (the
          + "License"); you may not use this file except in compliance
          + with the License. You may obtain a copy of the License at
          +
          + http://www.apache.org/licenses/LICENSE-2.0
          +
          + Unless required by applicable law or agreed to in writing,
          + software distributed under the License is distributed on an
          + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
          + KIND, either express or implied. See the License for the
          + specific language governing permissions and limitations
          + under the License.
          + */
          +
          +var path = require('path'),
          + fs = require('fs'),
          + cordovaUtil = require('../cordova/util'),
          + events = require('../events'),
          + Q = require('q'),
          + plugin = require('../cordova/plugin'),
          + PluginInfo = require('../PluginInfo'),
          + ConfigParser = require('../configparser/ConfigParser'),
          + CordovaError = require('../CordovaError'),
          + Context = require('./Context');
          +
          +/**
          + * Implements logic to retrieve hook script files defined in special folders and configuration
          + * files: config.xml, hooks/hook_type, plugins/../plugin.xml, etc
          + */
          +module.exports = {
          + /**
          + * Returns all script files for the hook type specified.
          + */
          + getHookScripts: function(hook, opts) {
          + // args check
          + if (!hook)

          { + throw new CordovaError('hook type is not specified'); + }

          + return getApplicationHookScripts(hook, opts)
          + .concat(getPluginsHookScripts(hook, opts));
          + }
          +};
          +
          +/**
          + * Returns script files defined on application level.
          + * They are stored in .cordova/hooks folders and in config.xml.
          + */
          +function getApplicationHookScripts(hook, opts) {
          + // args check
          + if (!hook)

          { + throw new CordovaError('hook type is not specified'); + }

          + return getApplicationHookScriptsFromDir(path.join(opts.projectRoot, '.cordova', 'hooks', hook))
          + .concat(getApplicationHookScriptsFromDir(path.join(opts.projectRoot, 'hooks', hook)))
          + .concat(getScriptsFromConfigXml(hook, opts));
          +}
          +
          +/**
          + * Returns script files defined by plugin developers as part of plugin.xml.
          + */
          +function getPluginsHookScripts(hook, opts) {
          + // args check
          + if (!hook) {
          + throw new CordovaError('hook type is not specified');
          — End diff –

          Got it, thanks!

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/55#discussion_r14822338 — Diff: cordova-lib/src/hooks/ScriptsFinder.js — @@ -0,0 +1,158 @@ +/** + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. + */ + +var path = require('path'), + fs = require('fs'), + cordovaUtil = require('../cordova/util'), + events = require('../events'), + Q = require('q'), + plugin = require('../cordova/plugin'), + PluginInfo = require('../PluginInfo'), + ConfigParser = require('../configparser/ConfigParser'), + CordovaError = require('../CordovaError'), + Context = require('./Context'); + +/** + * Implements logic to retrieve hook script files defined in special folders and configuration + * files: config.xml, hooks/hook_type, plugins/../plugin.xml, etc + */ +module.exports = { + /** + * Returns all script files for the hook type specified. + */ + getHookScripts: function(hook, opts) { + // args check + if (!hook) { + throw new CordovaError('hook type is not specified'); + } + return getApplicationHookScripts(hook, opts) + .concat(getPluginsHookScripts(hook, opts)); + } +}; + +/** + * Returns script files defined on application level. + * They are stored in .cordova/hooks folders and in config.xml. + */ +function getApplicationHookScripts(hook, opts) { + // args check + if (!hook) { + throw new CordovaError('hook type is not specified'); + } + return getApplicationHookScriptsFromDir(path.join(opts.projectRoot, '.cordova', 'hooks', hook)) + .concat(getApplicationHookScriptsFromDir(path.join(opts.projectRoot, 'hooks', hook))) + .concat(getScriptsFromConfigXml(hook, opts)); +} + +/** + * Returns script files defined by plugin developers as part of plugin.xml. + */ +function getPluginsHookScripts(hook, opts) { + // args check + if (!hook) { + throw new CordovaError('hook type is not specified'); — End diff – Got it, thanks!
          githubbot ASF GitHub Bot added a comment -

          Github user kamrik commented on a diff in the pull request:

          https://github.com/apache/cordova-lib/pull/55#discussion_r15655705

          — Diff: cordova-lib/src/hooks/scriptsFinder.js —
          @@ -0,0 +1,164 @@
          +/**
          + Licensed to the Apache Software Foundation (ASF) under one
          + or more contributor license agreements. See the NOTICE file
          + distributed with this work for additional information
          + regarding copyright ownership. The ASF licenses this file
          + to you under the Apache License, Version 2.0 (the
          + "License"); you may not use this file except in compliance
          + with the License. You may obtain a copy of the License at
          +
          + http://www.apache.org/licenses/LICENSE-2.0
          +
          + Unless required by applicable law or agreed to in writing,
          + software distributed under the License is distributed on an
          + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
          + KIND, either express or implied. See the License for the
          + specific language governing permissions and limitations
          + under the License.
          + */
          +
          +var path = require('path'),
          + fs = require('fs'),
          + cordovaUtil = require('../cordova/util'),
          + events = require('../events'),
          + Q = require('q'),
          + plugin = require('../cordova/plugin'),
          + PluginInfo = require('../PluginInfo'),
          + ConfigParser = require('../configparser/ConfigParser');
          +
          +/**
          + * Implements logic to retrieve hook script files defined in special folders and configuration
          + * files: config.xml, hooks/hook_type, plugins/../plugin.xml, etc
          + */
          +module.exports = {
          + /**
          + * Returns all script files for the hook type specified.
          + */
          + getHookScripts: function(hook, opts) {
          + // args check
          + if (!hook)

          { + throw new Error('hook type is not specified'); + }

          + return getApplicationHookScripts(hook, opts)
          + .concat(getPluginsHookScripts(hook, opts));
          + }
          +};
          +
          +/**
          + * Returns script files defined on application level.
          + * They are stored in .cordova/hooks folders and in config.xml.
          + */
          +function getApplicationHookScripts(hook, opts) {
          + // args check
          + if (!hook)

          { + throw new Error('hook type is not specified'); + }
          + return getApplicationHookScriptsFromDir(path.join(opts.projectRoot, '.cordova', 'hooks', hook))
          + .concat(getApplicationHookScriptsFromDir(path.join(opts.projectRoot, 'hooks', hook)))
          + .concat(getScriptsFromConfigXml(hook, opts));
          +}
          +
          +/**
          + * Returns script files defined by plugin developers as part of plugin.xml.
          + */
          +function getPluginsHookScripts(hook, opts) {
          + // args check
          + if (!hook) { + throw new Error('hook type is not specified'); + }

          +
          + // In case before_plugin_install, after_plugin_install, before_plugin_uninstall hooks we receive opts.plugin and
          + // retrieve scripts exclusive for this plugin.
          + if(opts.plugin)

          { + events.emit('debug', 'Executing "' + hook + '" hook for "' + opts.plugin.id + '" on ' + opts.plugin.platform + '.'); + + return getPluginScriptFiles(opts.plugin, hook, [ opts.plugin.platform ]); + }

          +
          + events.emit('debug', 'Executing "' + hook + '" hook for all plugins.');
          + return getAllPluginsHookScriptFiles(hook, opts);
          +}
          +
          +/**
          + * Gets application level hooks from the directrory specified.
          + */
          +function getApplicationHookScriptsFromDir(dir) {
          + if (!(fs.existsSync(dir)))

          { + return []; + }

          +
          + var compareNumbers = function(a, b)

          { + // TODO SG looks very complex, do we really need this? + return isNaN (parseInt(a, 10)) ? a.toLowerCase().localeCompare(b.toLowerCase ? b.toLowerCase(): b) + : parseInt(a, 10) > parseInt(b, 10) ? 1 : parseInt(a, 10) < parseInt(b, 10) ? -1 : 0; + }

          ;
          +
          + var scripts = fs.readdirSync(dir).sort(compareNumbers).filter(function(s)

          { + return s[0] != '.'; + }

          );
          + return scripts.map(function (scriptPath) {
          + // for old style hook files we don't use module loader for backward compatibility
          + return

          { path: scriptPath, fullPath: path.join(dir, scriptPath), useModuleLoader: false }

          ;
          + });
          +}
          +
          +/**
          + * Gets all scripts defined in config.xml with the specified type and platforms.
          + */
          +function getScriptsFromConfigXml(hook, opts) {
          + var configPath = cordovaUtil.projectConfig(opts.projectRoot);
          + var configXml;
          +
          + try

          { + configXml = new ConfigParser(configPath); + }

          catch(ex) {
          + events.emit('err', 'scriptsFinder could not load config.xml: ' + ex.message);
          + console.log('scriptsFinder could not load config.xml: ' + ex.message);
          + return [];
          — End diff –

          Why do we want a missing or bad config.xml go by silently? Are there any legit cases where this would run in a project with no config.xml file?

          By the way, what's the state of this PR? The code looks pretty good (just skimmed through it, didn't read thoroughly).

          githubbot ASF GitHub Bot added a comment - Github user kamrik commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/55#discussion_r15655705 — Diff: cordova-lib/src/hooks/scriptsFinder.js — @@ -0,0 +1,164 @@ +/** + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. + */ + +var path = require('path'), + fs = require('fs'), + cordovaUtil = require('../cordova/util'), + events = require('../events'), + Q = require('q'), + plugin = require('../cordova/plugin'), + PluginInfo = require('../PluginInfo'), + ConfigParser = require('../configparser/ConfigParser'); + +/** + * Implements logic to retrieve hook script files defined in special folders and configuration + * files: config.xml, hooks/hook_type, plugins/../plugin.xml, etc + */ +module.exports = { + /** + * Returns all script files for the hook type specified. + */ + getHookScripts: function(hook, opts) { + // args check + if (!hook) { + throw new Error('hook type is not specified'); + } + return getApplicationHookScripts(hook, opts) + .concat(getPluginsHookScripts(hook, opts)); + } +}; + +/** + * Returns script files defined on application level. + * They are stored in .cordova/hooks folders and in config.xml. + */ +function getApplicationHookScripts(hook, opts) { + // args check + if (!hook) { + throw new Error('hook type is not specified'); + } + return getApplicationHookScriptsFromDir(path.join(opts.projectRoot, '.cordova', 'hooks', hook)) + .concat(getApplicationHookScriptsFromDir(path.join(opts.projectRoot, 'hooks', hook))) + .concat(getScriptsFromConfigXml(hook, opts)); +} + +/** + * Returns script files defined by plugin developers as part of plugin.xml. + */ +function getPluginsHookScripts(hook, opts) { + // args check + if (!hook) { + throw new Error('hook type is not specified'); + } + + // In case before_plugin_install, after_plugin_install, before_plugin_uninstall hooks we receive opts.plugin and + // retrieve scripts exclusive for this plugin. + if(opts.plugin) { + events.emit('debug', 'Executing "' + hook + '" hook for "' + opts.plugin.id + '" on ' + opts.plugin.platform + '.'); + + return getPluginScriptFiles(opts.plugin, hook, [ opts.plugin.platform ]); + } + + events.emit('debug', 'Executing "' + hook + '" hook for all plugins.'); + return getAllPluginsHookScriptFiles(hook, opts); +} + +/** + * Gets application level hooks from the directrory specified. + */ +function getApplicationHookScriptsFromDir(dir) { + if (!(fs.existsSync(dir))) { + return []; + } + + var compareNumbers = function(a, b) { + // TODO SG looks very complex, do we really need this? + return isNaN (parseInt(a, 10)) ? a.toLowerCase().localeCompare(b.toLowerCase ? b.toLowerCase(): b) + : parseInt(a, 10) > parseInt(b, 10) ? 1 : parseInt(a, 10) < parseInt(b, 10) ? -1 : 0; + } ; + + var scripts = fs.readdirSync(dir).sort(compareNumbers).filter(function(s) { + return s[0] != '.'; + } ); + return scripts.map(function (scriptPath) { + // for old style hook files we don't use module loader for backward compatibility + return { path: scriptPath, fullPath: path.join(dir, scriptPath), useModuleLoader: false } ; + }); +} + +/** + * Gets all scripts defined in config.xml with the specified type and platforms. + */ +function getScriptsFromConfigXml(hook, opts) { + var configPath = cordovaUtil.projectConfig(opts.projectRoot); + var configXml; + + try { + configXml = new ConfigParser(configPath); + } catch(ex) { + events.emit('err', 'scriptsFinder could not load config.xml: ' + ex.message); + console.log('scriptsFinder could not load config.xml: ' + ex.message); + return []; — End diff – Why do we want a missing or bad config.xml go by silently? Are there any legit cases where this would run in a project with no config.xml file? By the way, what's the state of this PR? The code looks pretty good (just skimmed through it, didn't read thoroughly).
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on a diff in the pull request:

          https://github.com/apache/cordova-lib/pull/55#discussion_r15658798

          — Diff: cordova-lib/src/hooks/scriptsFinder.js —
          @@ -0,0 +1,164 @@
          +/**
          + Licensed to the Apache Software Foundation (ASF) under one
          + or more contributor license agreements. See the NOTICE file
          + distributed with this work for additional information
          + regarding copyright ownership. The ASF licenses this file
          + to you under the Apache License, Version 2.0 (the
          + "License"); you may not use this file except in compliance
          + with the License. You may obtain a copy of the License at
          +
          + http://www.apache.org/licenses/LICENSE-2.0
          +
          + Unless required by applicable law or agreed to in writing,
          + software distributed under the License is distributed on an
          + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
          + KIND, either express or implied. See the License for the
          + specific language governing permissions and limitations
          + under the License.
          + */
          +
          +var path = require('path'),
          + fs = require('fs'),
          + cordovaUtil = require('../cordova/util'),
          + events = require('../events'),
          + Q = require('q'),
          + plugin = require('../cordova/plugin'),
          + PluginInfo = require('../PluginInfo'),
          + ConfigParser = require('../configparser/ConfigParser');
          +
          +/**
          + * Implements logic to retrieve hook script files defined in special folders and configuration
          + * files: config.xml, hooks/hook_type, plugins/../plugin.xml, etc
          + */
          +module.exports = {
          + /**
          + * Returns all script files for the hook type specified.
          + */
          + getHookScripts: function(hook, opts) {
          + // args check
          + if (!hook)

          { + throw new Error('hook type is not specified'); + }

          + return getApplicationHookScripts(hook, opts)
          + .concat(getPluginsHookScripts(hook, opts));
          + }
          +};
          +
          +/**
          + * Returns script files defined on application level.
          + * They are stored in .cordova/hooks folders and in config.xml.
          + */
          +function getApplicationHookScripts(hook, opts) {
          + // args check
          + if (!hook)

          { + throw new Error('hook type is not specified'); + }
          + return getApplicationHookScriptsFromDir(path.join(opts.projectRoot, '.cordova', 'hooks', hook))
          + .concat(getApplicationHookScriptsFromDir(path.join(opts.projectRoot, 'hooks', hook)))
          + .concat(getScriptsFromConfigXml(hook, opts));
          +}
          +
          +/**
          + * Returns script files defined by plugin developers as part of plugin.xml.
          + */
          +function getPluginsHookScripts(hook, opts) {
          + // args check
          + if (!hook) { + throw new Error('hook type is not specified'); + }

          +
          + // In case before_plugin_install, after_plugin_install, before_plugin_uninstall hooks we receive opts.plugin and
          + // retrieve scripts exclusive for this plugin.
          + if(opts.plugin)

          { + events.emit('debug', 'Executing "' + hook + '" hook for "' + opts.plugin.id + '" on ' + opts.plugin.platform + '.'); + + return getPluginScriptFiles(opts.plugin, hook, [ opts.plugin.platform ]); + }

          +
          + events.emit('debug', 'Executing "' + hook + '" hook for all plugins.');
          + return getAllPluginsHookScriptFiles(hook, opts);
          +}
          +
          +/**
          + * Gets application level hooks from the directrory specified.
          + */
          +function getApplicationHookScriptsFromDir(dir) {
          + if (!(fs.existsSync(dir)))

          { + return []; + }

          +
          + var compareNumbers = function(a, b)

          { + // TODO SG looks very complex, do we really need this? + return isNaN (parseInt(a, 10)) ? a.toLowerCase().localeCompare(b.toLowerCase ? b.toLowerCase(): b) + : parseInt(a, 10) > parseInt(b, 10) ? 1 : parseInt(a, 10) < parseInt(b, 10) ? -1 : 0; + }

          ;
          +
          + var scripts = fs.readdirSync(dir).sort(compareNumbers).filter(function(s)

          { + return s[0] != '.'; + }

          );
          + return scripts.map(function (scriptPath) {
          + // for old style hook files we don't use module loader for backward compatibility
          + return

          { path: scriptPath, fullPath: path.join(dir, scriptPath), useModuleLoader: false }

          ;
          + });
          +}
          +
          +/**
          + * Gets all scripts defined in config.xml with the specified type and platforms.
          + */
          +function getScriptsFromConfigXml(hook, opts) {
          + var configPath = cordovaUtil.projectConfig(opts.projectRoot);
          + var configXml;
          +
          + try

          { + configXml = new ConfigParser(configPath); + }

          catch(ex) {
          + events.emit('err', 'scriptsFinder could not load config.xml: ' + ex.message);
          + console.log('scriptsFinder could not load config.xml: ' + ex.message);
          + return [];
          — End diff –

          This is done sine some unit tests does not have config.xml. This will be updated tomorrow to rely on spyon to have this place clean and shiny.
          Otherwise this feature is fully ready, all previous reviews noret have been addressed.

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/55#discussion_r15658798 — Diff: cordova-lib/src/hooks/scriptsFinder.js — @@ -0,0 +1,164 @@ +/** + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. + */ + +var path = require('path'), + fs = require('fs'), + cordovaUtil = require('../cordova/util'), + events = require('../events'), + Q = require('q'), + plugin = require('../cordova/plugin'), + PluginInfo = require('../PluginInfo'), + ConfigParser = require('../configparser/ConfigParser'); + +/** + * Implements logic to retrieve hook script files defined in special folders and configuration + * files: config.xml, hooks/hook_type, plugins/../plugin.xml, etc + */ +module.exports = { + /** + * Returns all script files for the hook type specified. + */ + getHookScripts: function(hook, opts) { + // args check + if (!hook) { + throw new Error('hook type is not specified'); + } + return getApplicationHookScripts(hook, opts) + .concat(getPluginsHookScripts(hook, opts)); + } +}; + +/** + * Returns script files defined on application level. + * They are stored in .cordova/hooks folders and in config.xml. + */ +function getApplicationHookScripts(hook, opts) { + // args check + if (!hook) { + throw new Error('hook type is not specified'); + } + return getApplicationHookScriptsFromDir(path.join(opts.projectRoot, '.cordova', 'hooks', hook)) + .concat(getApplicationHookScriptsFromDir(path.join(opts.projectRoot, 'hooks', hook))) + .concat(getScriptsFromConfigXml(hook, opts)); +} + +/** + * Returns script files defined by plugin developers as part of plugin.xml. + */ +function getPluginsHookScripts(hook, opts) { + // args check + if (!hook) { + throw new Error('hook type is not specified'); + } + + // In case before_plugin_install, after_plugin_install, before_plugin_uninstall hooks we receive opts.plugin and + // retrieve scripts exclusive for this plugin. + if(opts.plugin) { + events.emit('debug', 'Executing "' + hook + '" hook for "' + opts.plugin.id + '" on ' + opts.plugin.platform + '.'); + + return getPluginScriptFiles(opts.plugin, hook, [ opts.plugin.platform ]); + } + + events.emit('debug', 'Executing "' + hook + '" hook for all plugins.'); + return getAllPluginsHookScriptFiles(hook, opts); +} + +/** + * Gets application level hooks from the directrory specified. + */ +function getApplicationHookScriptsFromDir(dir) { + if (!(fs.existsSync(dir))) { + return []; + } + + var compareNumbers = function(a, b) { + // TODO SG looks very complex, do we really need this? + return isNaN (parseInt(a, 10)) ? a.toLowerCase().localeCompare(b.toLowerCase ? b.toLowerCase(): b) + : parseInt(a, 10) > parseInt(b, 10) ? 1 : parseInt(a, 10) < parseInt(b, 10) ? -1 : 0; + } ; + + var scripts = fs.readdirSync(dir).sort(compareNumbers).filter(function(s) { + return s[0] != '.'; + } ); + return scripts.map(function (scriptPath) { + // for old style hook files we don't use module loader for backward compatibility + return { path: scriptPath, fullPath: path.join(dir, scriptPath), useModuleLoader: false } ; + }); +} + +/** + * Gets all scripts defined in config.xml with the specified type and platforms. + */ +function getScriptsFromConfigXml(hook, opts) { + var configPath = cordovaUtil.projectConfig(opts.projectRoot); + var configXml; + + try { + configXml = new ConfigParser(configPath); + } catch(ex) { + events.emit('err', 'scriptsFinder could not load config.xml: ' + ex.message); + console.log('scriptsFinder could not load config.xml: ' + ex.message); + return []; — End diff – This is done sine some unit tests does not have config.xml. This will be updated tomorrow to rely on spyon to have this place clean and shiny. Otherwise this feature is fully ready, all previous reviews noret have been addressed.
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-50883550

          Addressed @kamrik code review notes

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-50883550 Addressed @kamrik code review notes
          githubbot ASF GitHub Bot added a comment -

          Github user daserge commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-51382634

          Rebased to master and investigating issues with `uninstall-browserify.spec.js` now.
          `cordova.js` seems to be blocked inside fixture projects by `uninstall-browserify` so that `shell.rm` fails.

          githubbot ASF GitHub Bot added a comment - Github user daserge commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-51382634 Rebased to master and investigating issues with `uninstall-browserify.spec.js` now. `cordova.js` seems to be blocked inside fixture projects by `uninstall-browserify` so that `shell.rm` fails.
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-51389090

          btw, loooks like all tests are passed on both windows and linux

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-51389090 btw, loooks like all tests are passed on both windows and linux
          githubbot ASF GitHub Bot added a comment -

          Github user csantanapr commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-51601294

          @sgrebnov So far I have found one problem.
          When I have the hooks tags inside a platform, the second tag for "after_prepare" doesn't work.
          This is because options.plugin get set in the Context.opts when calling "before_prepare" affecting the search for scripts in "after_prepare"

          if the two tags are outside the <platform> tag then it finds the script, but inside the <platform> tag it doesn't

          create a plugin.xml like this:

          You will notice that iosAfterPrepare.js never runs

          <?xml version="1.0" encoding="UTF-8"?>
          <plugin xmlns="http://apache.org/cordova/ns/plugins/1.0"
          xmlns:android="http://schemas.android.com/apk/res/android"
          id="com.plugin.withhooks"
          version="0.0.1">
          <name>Plugin with hooks</name>

          <platform name="ios">
          <hook type="before_prepare" src="scripts/ios/iosBeforePrepare.js" />
          <hook type="after_prepare" src="scripts/ios/iosAfterPrepare.js" />
          </platform>

          </plugin>
          One way to fix it is doing a Copy in Context.js, and do a copy of opts, this way opts can be use in other places without the chance of being modify.
          Here is a change I did:
          https://github.com/csantanapr/cordova-lib/commit/a0ef4ef172f8818c3bc770899dba5dbab06fa03e#diff-d41d8cd98f00b204e9800998ecf8427e

          This might be one, avoid modifying context and doing a copy of opts:
          https://github.com/MSOpenTech/cordova-lib/blob/CB-6481-hooks/cordova-lib/src/hooks/HooksRunner.js#L161

          githubbot ASF GitHub Bot added a comment - Github user csantanapr commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-51601294 @sgrebnov So far I have found one problem. When I have the hooks tags inside a platform, the second tag for "after_prepare" doesn't work. This is because options.plugin get set in the Context.opts when calling "before_prepare" affecting the search for scripts in "after_prepare" if the two tags are outside the <platform> tag then it finds the script, but inside the <platform> tag it doesn't create a plugin.xml like this: You will notice that iosAfterPrepare.js never runs <?xml version="1.0" encoding="UTF-8"?> <plugin xmlns="http://apache.org/cordova/ns/plugins/1.0" xmlns:android="http://schemas.android.com/apk/res/android" id="com.plugin.withhooks" version="0.0.1"> <name>Plugin with hooks</name> <platform name="ios"> <hook type="before_prepare" src="scripts/ios/iosBeforePrepare.js" /> <hook type="after_prepare" src="scripts/ios/iosAfterPrepare.js" /> </platform> </plugin> One way to fix it is doing a Copy in Context.js, and do a copy of opts, this way opts can be use in other places without the chance of being modify. Here is a change I did: https://github.com/csantanapr/cordova-lib/commit/a0ef4ef172f8818c3bc770899dba5dbab06fa03e#diff-d41d8cd98f00b204e9800998ecf8427e This might be one, avoid modifying context and doing a copy of opts: https://github.com/MSOpenTech/cordova-lib/blob/CB-6481-hooks/cordova-lib/src/hooks/HooksRunner.js#L161
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-51718759

          Confirm this issue, @csantanapr I've cherry-picked your fix (Thx!), also added minor improvement. We may want to better formalize opts structure (special class?)

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-51718759 Confirm this issue, @csantanapr I've cherry-picked your fix (Thx!), also added minor improvement. We may want to better formalize opts structure (special class?)
          githubbot ASF GitHub Bot added a comment -

          Github user kamrik commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-51783588

          Problems with opts objects arise all over the place in cordova-lib, I got bitten by similar problems several times. My proposal would be to create a CordovaProject class that would hold all of the stuff like projectRoot, configuration based on command line arguments, the EventEmitter instance tor this project etc etc - all of the stuff that should be accessible globally.

          githubbot ASF GitHub Bot added a comment - Github user kamrik commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-51783588 Problems with opts objects arise all over the place in cordova-lib, I got bitten by similar problems several times. My proposal would be to create a CordovaProject class that would hold all of the stuff like projectRoot, configuration based on command line arguments, the EventEmitter instance tor this project etc etc - all of the stuff that should be accessible globally.
          githubbot ASF GitHub Bot added a comment -

          Github user csantanapr commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-51805610

          Yes I agree having a class, but still pass a single context to the hook
          and then the Context object contains a CordovaProject class object return by util.js I think this is the module that know more about the project if I recall corectly.

          githubbot ASF GitHub Bot added a comment - Github user csantanapr commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-51805610 Yes I agree having a class, but still pass a single context to the hook and then the Context object contains a CordovaProject class object return by util.js I think this is the module that know more about the project if I recall corectly.
          githubbot ASF GitHub Bot added a comment -

          Github user kamrik commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-51810029

          Unfortunately utils.js knows nothing about the project, it re-reads all the info from file system on each call. This results in lots of cases of re-parsing of config files or re-listing the same dirs over and over again. On top of that we add some caching, which then results in weird cache-invalidation bugs.
          As I see it, CordovaProject instance should be created by the CLI (or by any other consumer of cordova-lib) exactly once per process invocation, and then passed around to be available to almost any cordova-lib function.

          // Along the lines of
          var cordovaProject = new cordova_lib.CordovaProject(rootDir, ?)
          cordovaProject.verbose = args.verbose // etc.
          cordova.platform(cordovaProject, 'add', ['android'])
          // Or even
          crodovaProject.platform('add', ['android'])
          // And then inside cordova-lib we use
          hooksRunner = new HooksRunner(cordovaProject);

          In most places where we pass projectRoot, it should be replaced by cordovaProject. Or have it accessible as either "this" (e.t. in "create()") or "this.cordovaProject" (e.g. in HooksRunner methods).

          githubbot ASF GitHub Bot added a comment - Github user kamrik commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-51810029 Unfortunately utils.js knows nothing about the project, it re-reads all the info from file system on each call. This results in lots of cases of re-parsing of config files or re-listing the same dirs over and over again. On top of that we add some caching, which then results in weird cache-invalidation bugs. As I see it, CordovaProject instance should be created by the CLI (or by any other consumer of cordova-lib) exactly once per process invocation, and then passed around to be available to almost any cordova-lib function. // Along the lines of var cordovaProject = new cordova_lib.CordovaProject(rootDir, ?) cordovaProject.verbose = args.verbose // etc. cordova.platform(cordovaProject, 'add', ['android'] ) // Or even crodovaProject.platform('add', ['android'] ) // And then inside cordova-lib we use hooksRunner = new HooksRunner(cordovaProject); In most places where we pass projectRoot, it should be replaced by cordovaProject. Or have it accessible as either "this" (e.t. in "create()") or "this.cordovaProject" (e.g. in HooksRunner methods).
          githubbot ASF GitHub Bot added a comment -

          Github user agrieve commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-51811935

          +1 for the "or even" clause there. I'd like to be able to write:

          var cordovaProject = cordova_lib.loadProject(rootDir)
          cordovaProject.setVerbosity('verbose')
          cordovaProject.addPlatform('android') // returns a promise
          cordovaProject.addHook('pre-prepare', ...)

          githubbot ASF GitHub Bot added a comment - Github user agrieve commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-51811935 +1 for the "or even" clause there. I'd like to be able to write: var cordovaProject = cordova_lib.loadProject(rootDir) cordovaProject.setVerbosity('verbose') cordovaProject.addPlatform('android') // returns a promise cordovaProject.addHook('pre-prepare', ...)
          githubbot ASF GitHub Bot added a comment -

          Github user csantanapr commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-51839887

          So how do we move forward with plugin hooks?

          Do we land what we have here and then open a new JIRA to take care about cordovaProject object?

          githubbot ASF GitHub Bot added a comment - Github user csantanapr commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-51839887 So how do we move forward with plugin hooks? Do we land what we have here and then open a new JIRA to take care about cordovaProject object?
          githubbot ASF GitHub Bot added a comment -

          Github user csantanapr commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-51840027

          I mean land in terms of pushing to master for others to contribute, and when feature is ready document as public api/feature.

          githubbot ASF GitHub Bot added a comment - Github user csantanapr commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-51840027 I mean land in terms of pushing to master for others to contribute, and when feature is ready document as public api/feature.
          githubbot ASF GitHub Bot added a comment -

          Github user kamrik commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-51841422

          I think we should leave this decision to Sergey and the Microsoft guys. While it would be nice to have CordovaProject class right away, it might take quite some time and we shouldn't delay the plugin hooks because of this. +1 for a new JIRA issue for CordovaProject.

          githubbot ASF GitHub Bot added a comment - Github user kamrik commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-51841422 I think we should leave this decision to Sergey and the Microsoft guys. While it would be nice to have CordovaProject class right away, it might take quite some time and we shouldn't delay the plugin hooks because of this. +1 for a new JIRA issue for CordovaProject.
          githubbot ASF GitHub Bot added a comment -

          Github user csantanapr commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-51868008

          Here is a gist of a Hook I'm working on for IBM
          https://gist.github.com/csantanapr/9fc45c76b4d9a2d5ef85

          You can see how I'm using Context opts and requireCordovaModule
          2 Items I'm looking how to improve is:
          1. I want to be able to use nopt module, but I can't using requireCordovaModule, because nopt is a dep from cli not cordova-lib
          2. Getting a parser for the platform doing
          platforms = context.requireCordovaModule('../cordova/platforms')
          parser = new platforms[platformId].parser(platformPath);
          parser.www_dir()
          It feels like using a private API

          githubbot ASF GitHub Bot added a comment - Github user csantanapr commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-51868008 Here is a gist of a Hook I'm working on for IBM https://gist.github.com/csantanapr/9fc45c76b4d9a2d5ef85 You can see how I'm using Context opts and requireCordovaModule 2 Items I'm looking how to improve is: 1. I want to be able to use nopt module, but I can't using requireCordovaModule, because nopt is a dep from cli not cordova-lib 2. Getting a parser for the platform doing platforms = context.requireCordovaModule('../cordova/platforms') parser = new platforms [platformId] .parser(platformPath); parser.www_dir() It feels like using a private API
          githubbot ASF GitHub Bot added a comment -

          Github user csantanapr commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-51868071

          @sgrebnov How do I get the command line arguments out from Context?
          In none javascript interface this is located in CORDOVA_CMDLINE

          githubbot ASF GitHub Bot added a comment - Github user csantanapr commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-51868071 @sgrebnov How do I get the command line arguments out from Context? In none javascript interface this is located in CORDOVA_CMDLINE
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-51903709

          Thx for review and notes. +1000 to wrap parameters to special class instance, but I would prefer to push this version since it takes time to re-base it all over the time. Moving forward I can create a prototype implementation of CordovaProject if nobody has already started working on this.

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-51903709 Thx for review and notes. +1000 to wrap parameters to special class instance, but I would prefer to push this version since it takes time to re-base it all over the time. Moving forward I can create a prototype implementation of CordovaProject if nobody has already started working on this.
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-51904349

          Answering Carlos questions
          1. I believe recommended solution here is placing required dependencies along with plugin (node_modules folder inside your scripts folder)
          2. I see, we may want to remove requireCordovaModule and just put all things we want to expose to context
          3. cmd string should be accessible as context.cmdLine

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-51904349 Answering Carlos questions 1. I believe recommended solution here is placing required dependencies along with plugin (node_modules folder inside your scripts folder) 2. I see, we may want to remove requireCordovaModule and just put all things we want to expose to context 3. cmd string should be accessible as context.cmdLine
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-51905210

          + sharing some my experiments with plugin hooks: WP8 JavaScript debugger implemented as cordova plugin
          https://github.com/sgrebnov/cordova-debug-wp8
          https://vimeo.com/102415578

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-51905210 + sharing some my experiments with plugin hooks: WP8 JavaScript debugger implemented as cordova plugin https://github.com/sgrebnov/cordova-debug-wp8 https://vimeo.com/102415578
          githubbot ASF GitHub Bot added a comment -

          Github user csantanapr commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-51937234

          @sgrebnov I agree now that plugin hooks are located in a unique place like plugins/mypluginid/src I can have my node module dependencies isolated without impacting others. and now thinking about user needing to do npm install, I think the node modules should be a short list so I can include node_modules directory with the modules already installed with my plugin.

          I just tried var nopt = context.requireCordovaModule('nopt');

          maybe I did a typo before, when it was not working. so far I'm trying to keep the hook simple without external dependencies.

          on requireCordovaModule I think we should keep it for now, it's a very good backdoor we can use it during development to come up with what set of things we want to expose.

          For now I need a parser, so adding context.opts.parsers =[

          {ios:}

          ,

          {android:}

          ] and array with parser might be a good thing.

          about context.cmdLine I see it now, I must be blind :-p

          Go ahead and work CordovaProject, let me know if you need help or just feedback.

          I will continue to work on the hook for IBM, and add test cases for in cordova-lib as I find use cases we want to cover in unit tests

          githubbot ASF GitHub Bot added a comment - Github user csantanapr commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-51937234 @sgrebnov I agree now that plugin hooks are located in a unique place like plugins/mypluginid/src I can have my node module dependencies isolated without impacting others. and now thinking about user needing to do npm install, I think the node modules should be a short list so I can include node_modules directory with the modules already installed with my plugin. I just tried var nopt = context.requireCordovaModule('nopt'); maybe I did a typo before, when it was not working. so far I'm trying to keep the hook simple without external dependencies. on requireCordovaModule I think we should keep it for now, it's a very good backdoor we can use it during development to come up with what set of things we want to expose. For now I need a parser, so adding context.opts.parsers =[ {ios:} , {android:} ] and array with parser might be a good thing. about context.cmdLine I see it now, I must be blind :-p Go ahead and work CordovaProject, let me know if you need help or just feedback. I will continue to work on the hook for IBM, and add test cases for in cordova-lib as I find use cases we want to cover in unit tests
          githubbot ASF GitHub Bot added a comment -

          Github user csantanapr commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-52238594

          @sgrebnov here some feedback about context.cmdLine

          Any strong reason why this has to be a string instead of an Array like process.argv ?

          https://github.com/MSOpenTech/cordova-lib/blob/CB-6481-hooks/cordova-lib/src/hooks/Context.js#L44

          this.cmdLine = process.argv.join(' ');

          I want to parse but I don't feel comfortable doing a split(' ') on spaces because the parameter I'm looking for is a path and path can have spaces specially in Windows

          Can we make context.cmdLine be process.argv and not a String or add a context.cmdArgv to Context Class.

          This is what I have and don't like the idea of converting to Array with split on spaces

          ````
          var cmdLine = context.cmdLine.split(' ').filter(function(item)

          { return item !== '--'; }

          );
          var knowOpts =

          {'wlpath': path}

          ;
          var shortHands =

          {'wl' : '--wlpath'}

          ;
          var parsedCmdLine = nopt(knowOpts, shortHands, cmdLine, 0);

          console.log(parsedCmdLine);
          if(parsedCmdLine.wlpath)

          { console.log('Updating Worklight App path to;'+parsedCmdLine.wlpath); wlappPath = parsedCmdLine.wlpath; }

          ````
          Oh one thing weird about using nopt is if dash dash <space> '-- ' is present nopt doesn't parse correctly, that's why I'm using a filter

          Let me know what you think

          githubbot ASF GitHub Bot added a comment - Github user csantanapr commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-52238594 @sgrebnov here some feedback about context.cmdLine Any strong reason why this has to be a string instead of an Array like process.argv ? https://github.com/MSOpenTech/cordova-lib/blob/CB-6481-hooks/cordova-lib/src/hooks/Context.js#L44 this.cmdLine = process.argv.join(' '); I want to parse but I don't feel comfortable doing a split(' ') on spaces because the parameter I'm looking for is a path and path can have spaces specially in Windows Can we make context.cmdLine be process.argv and not a String or add a context.cmdArgv to Context Class. This is what I have and don't like the idea of converting to Array with split on spaces ```` var cmdLine = context.cmdLine.split(' ').filter(function(item) { return item !== '--'; } ); var knowOpts = {'wlpath': path} ; var shortHands = {'wl' : '--wlpath'} ; var parsedCmdLine = nopt(knowOpts, shortHands, cmdLine, 0); console.log(parsedCmdLine); if(parsedCmdLine.wlpath) { console.log('Updating Worklight App path to;'+parsedCmdLine.wlpath); wlappPath = parsedCmdLine.wlpath; } ```` Oh one thing weird about using nopt is if dash dash <space> '-- ' is present nopt doesn't parse correctly, that's why I'm using a filter Let me know what you think
          githubbot ASF GitHub Bot added a comment -

          Github user purplecabbage commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-52243131

          @csantanapr
          If your hook-script is node, it will be loaded as a module, so you still have access to process.argv don't you?

          githubbot ASF GitHub Bot added a comment - Github user purplecabbage commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-52243131 @csantanapr If your hook-script is node, it will be loaded as a module, so you still have access to process.argv don't you?
          githubbot ASF GitHub Bot added a comment -

          Github user purplecabbage commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-52261451

          Thinking about this some more, I think we should just pass ```context.cmdLine = process.argv.slice()``` or perhaps more unambiguously ```context.proc_argv = process.argv.slice()```
          This way we can still mock the interface for tests.

          githubbot ASF GitHub Bot added a comment - Github user purplecabbage commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-52261451 Thinking about this some more, I think we should just pass ```context.cmdLine = process.argv.slice()``` or perhaps more unambiguously ```context.proc_argv = process.argv.slice()``` This way we can still mock the interface for tests.
          githubbot ASF GitHub Bot added a comment -

          Github user csantanapr commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-52305148

          @purplecabbage That was one of my points, that context.cmdLine is not useful as a String.
          Why slide() and not just context.cmdLine = process.argv ? just to get a shallow copy?

          Not part of this pull request, but I think no where in inside cordova-lib the global object "process.argv" should be assume to be use.
          I think is a responsibility of of who ever is calling cordova-lib which in this case is cordova-cli here:
          https://github.com/apache/cordova-cli/blob/master/bin/cordova#L41

          addTs('start');
          var cli = require('../src/cli');
          cli(process.argv);
          addTs('end');

          then in ../src/cli
          should parse and pass down remains/noneparsed of inputArgs down to the cordova-lib command.

          But for now for this pull request I +1 to do context.proc_argv = process.argv.slice()

          @sgrebnov you going to merge this pull request into master soon?

          githubbot ASF GitHub Bot added a comment - Github user csantanapr commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-52305148 @purplecabbage That was one of my points, that context.cmdLine is not useful as a String. Why slide() and not just context.cmdLine = process.argv ? just to get a shallow copy? Not part of this pull request, but I think no where in inside cordova-lib the global object "process.argv" should be assume to be use. I think is a responsibility of of who ever is calling cordova-lib which in this case is cordova-cli here: https://github.com/apache/cordova-cli/blob/master/bin/cordova#L41 addTs('start'); var cli = require('../src/cli'); cli(process.argv); addTs('end'); then in ../src/cli should parse and pass down remains/noneparsed of inputArgs down to the cordova-lib command. But for now for this pull request I +1 to do context.proc_argv = process.argv.slice() @sgrebnov you going to merge this pull request into master soon?
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-56843816

          rebased on top of apache/master; I see some unit tests are failed now, investigating ..

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-56843816 rebased on top of apache/master; I see some unit tests are failed now, investigating ..
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-56856482

          I compare tests results with results of master branch and see exactly the same tests failed, looks like this code does not add new failed tests

          master
          https://travis-ci.org/apache/cordova-lib/builds/36205938

          hooks
          https://travis-ci.org/apache/cordova-lib/builds/36274570

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-56856482 I compare tests results with results of master branch and see exactly the same tests failed, looks like this code does not add new failed tests master https://travis-ci.org/apache/cordova-lib/builds/36205938 hooks https://travis-ci.org/apache/cordova-lib/builds/36274570
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-56869833

          I propose to merge this version since looks like it provides all basic functionality we want for unified hooks. If so, could someone review and merge

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-56869833 I propose to merge this version since looks like it provides all basic functionality we want for unified hooks. If so, could someone review and merge
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-56869972

          All further changes and improvements we can file as separate JIRA issues/tasks

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-56869972 All further changes and improvements we can file as separate JIRA issues/tasks
          githubbot ASF GitHub Bot added a comment -

          Github user csantanapr commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-56910355

          I will review the pull request and if it looks you want me to merge it or you want to do it @sgrebnov ?

          githubbot ASF GitHub Bot added a comment - Github user csantanapr commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-56910355 I will review the pull request and if it looks you want me to merge it or you want to do it @sgrebnov ?
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-56928367

          Thanks, Carlos; I'm totally fine with either you or me merge it, just want someone else to review the code before pushing it to master

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-56928367 Thanks, Carlos; I'm totally fine with either you or me merge it, just want someone else to review the code before pushing it to master
          githubbot ASF GitHub Bot added a comment -

          Github user csantanapr commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-57020278

          @sgrebnov I tested the pull request and it looks good :+1: . Go ahead and merge it into master. this is great extensibility feature :beers: :tophat:

          githubbot ASF GitHub Bot added a comment - Github user csantanapr commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-57020278 @sgrebnov I tested the pull request and it looks good :+1: . Go ahead and merge it into master. this is great extensibility feature :beers: :tophat:

          Commit a3169336634508246754b8e52bd452edba5ecc7f in cordova-lib's branch refs/heads/master from daserge
          [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=a316933 ]

          CB-6481 Fixed HooksRunner and tests
          Avoided issue with parallel tests running
          Added checks for handling mocked config.xml and package.json in HooksRunner and scriptsFinder
          Addressed jshint issues
          Renamed ScriptsFinder to scriptsFinder

          jira-bot ASF subversion and git services added a comment - Commit a3169336634508246754b8e52bd452edba5ecc7f in cordova-lib's branch refs/heads/master from daserge [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=a316933 ] CB-6481 Fixed HooksRunner and tests Avoided issue with parallel tests running Added checks for handling mocked config.xml and package.json in HooksRunner and scriptsFinder Addressed jshint issues Renamed ScriptsFinder to scriptsFinder

          Commit 1b5ce69b93833145f14772213600c0e516ccff64 in cordova-lib's branch refs/heads/master from daserge
          [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=1b5ce69 ]

          CB-6481 Fixed tests - removed output

          Avoided tests output - mocked hooker fire in windows8 and wp8 tests
          Added pwd check missing case to isCordova
          Removed exception swallowing in case config.xml is not found in hooks/scriptFinder

          jira-bot ASF subversion and git services added a comment - Commit 1b5ce69b93833145f14772213600c0e516ccff64 in cordova-lib's branch refs/heads/master from daserge [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=1b5ce69 ] CB-6481 Fixed tests - removed output Avoided tests output - mocked hooker fire in windows8 and wp8 tests Added pwd check missing case to isCordova Removed exception swallowing in case config.xml is not found in hooks/scriptFinder

          Commit b74d87d8ef1f2f5a7c967e50cdb3d177119833bc in cordova-lib's branch refs/heads/master from Carlos Santana
          [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=b74d87d ]

          CB-6481 Context opts should copy not reference

          jira-bot ASF subversion and git services added a comment - Commit b74d87d8ef1f2f5a7c967e50cdb3d177119833bc in cordova-lib's branch refs/heads/master from Carlos Santana [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=b74d87d ] CB-6481 Context opts should copy not reference

          Commit a8cf9fd23dd74b897e45de6b4f94a796885f9b7c in cordova-lib's branch refs/heads/master from sgrebnov
          [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=a8cf9fd ]

          CB-6481 getPluginsHookScripts to work if plugin platform not defined

          Also HookRunner minor code improvements

          jira-bot ASF subversion and git services added a comment - Commit a8cf9fd23dd74b897e45de6b4f94a796885f9b7c in cordova-lib's branch refs/heads/master from sgrebnov [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=a8cf9fd ] CB-6481 getPluginsHookScripts to work if plugin platform not defined Also HookRunner minor code improvements

          Commit 57f454b2875fd429d1b02da5bd5c3196d77363e6 in cordova-lib's branch refs/heads/master from daserge
          [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=57f454b ]

          CB-6481 Fixed comment

          jira-bot ASF subversion and git services added a comment - Commit 57f454b2875fd429d1b02da5bd5c3196d77363e6 in cordova-lib's branch refs/heads/master from daserge [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=57f454b ] CB-6481 Fixed comment
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/cordova-lib/pull/55

          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/cordova-lib/pull/55
          githubbot ASF GitHub Bot added a comment -

          Github user sgrebnov commented on the pull request:

          https://github.com/apache/cordova-lib/pull/55#issuecomment-57022338

          merged, thx @csantanapr!

          githubbot ASF GitHub Bot added a comment - Github user sgrebnov commented on the pull request: https://github.com/apache/cordova-lib/pull/55#issuecomment-57022338 merged, thx @csantanapr!
          jsoref Josh Soref added a comment -

          This broke:

          cordova platform check

          jsoref Josh Soref added a comment - This broke: cordova platform check
          githubbot ASF GitHub Bot added a comment -

          GitHub user jsoref opened a pull request:

          https://github.com/apache/cordova-lib/pull/107

          CB-6481 (Fix cordova platform check)

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/jsoref/cordova-lib cb_6481

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/cordova-lib/pull/107.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #107


          commit 58b37f5bf010a62c882ab3acfb23b69a41c1710e
          Author: Josh Soref <jsoref@blackberry.com>
          Date: 2014-10-21T00:51:30Z

          CB-6481 (Fix cordova platform check)


          githubbot ASF GitHub Bot added a comment - GitHub user jsoref opened a pull request: https://github.com/apache/cordova-lib/pull/107 CB-6481 (Fix cordova platform check) You can merge this pull request into a Git repository by running: $ git pull https://github.com/jsoref/cordova-lib cb_6481 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cordova-lib/pull/107.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #107 commit 58b37f5bf010a62c882ab3acfb23b69a41c1710e Author: Josh Soref <jsoref@blackberry.com> Date: 2014-10-21T00:51:30Z CB-6481 (Fix cordova platform check)

          Commit 58b37f5bf010a62c882ab3acfb23b69a41c1710e in cordova-lib's branch refs/heads/master from Josh Soref
          [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=58b37f5 ]

          CB-6481 (Fix cordova platform check)

          jira-bot ASF subversion and git services added a comment - Commit 58b37f5bf010a62c882ab3acfb23b69a41c1710e in cordova-lib's branch refs/heads/master from Josh Soref [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=58b37f5 ] CB-6481 (Fix cordova platform check)
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/cordova-lib/pull/107

          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/cordova-lib/pull/107

          People

            sgrebnov Sergey Grebnov
            sgrebnov Sergey Grebnov
            Votes:
            0 Vote for this issue
            Watchers:
            Stop watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Slack

                Issue deployment

                  Client must be authenticated to access this resource.