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
- relates to
-
CB-7718 CordovaLib fails when installing transitive dependencies
- Closed
Activity
Github user sgrebnov commented on the pull request:
https://github.com/apache/cordova-plugman/pull/74#issuecomment-41850345
Switched to single parameter called context
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.
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)
+
+ events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.');
+
+ var scriptTypes = getScriptTypesForHook(type);
+ if (scriptTypes == null)
+
+ 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.
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)
+
+ events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.');
+
+ var scriptTypes = getScriptTypesForHook(type);
+ if (scriptTypes == null)
+
+ var scriptFilesToRun = getScriptFiles(pluginElement, scriptTypes, platform);
+ context =
;
+
+ 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 =
+
+ 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)
+ });
+ 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.
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)
+
+ events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.');
+
+ var scriptTypes = getScriptTypesForHook(type);
+ if (scriptTypes == null)
+
+ var scriptFilesToRun = getScriptFiles(pluginElement, scriptTypes, platform);
+ context =
;
+
+ 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 =
+
+ 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)
+ });
+ return scriptFiles;
+}
+
+/**
+ * Async runs the script files.
+ */
+function runScripts(scripts, context) {
+ var pendingScripts = [];
+
+ scripts.forEach(function(script)
);
+
+ return Q.all(pendingScripts);
+};
+
+/**
+ * Async runs single script file.
+ */
+function runScriptFile(scriptPath, context) {
+
+ scriptPath = path.join(context.pluginDir, scriptPath);
+
+ if(!fs.existsSync(scriptPath))
+ 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?
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
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)
+
+ 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)`
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)
+
+ events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.');
+
+ var scriptTypes = getScriptTypesForHook(type);
+ if (scriptTypes == null)
+
+ var scriptFilesToRun = getScriptFiles(pluginElement, scriptTypes, platform);
+ var context =
;
+
+ 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 =
+
+ 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?
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)
+
+ events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.');
+
+ var scriptTypes = getScriptTypesForHook(type);
+ if (scriptTypes == null)
+
+ var scriptFilesToRun = getScriptFiles(pluginElement, scriptTypes, platform);
+ var context =
;
+
+ 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 =
+
+ 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?
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)
+
+ events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.');
+
+ var scriptTypes = getScriptTypesForHook(type);
+ if (scriptTypes == null)
+
+ var scriptFilesToRun = getScriptFiles(pluginElement, scriptTypes, platform);
+ var context =
;
+
+ 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 =
+
+ 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.
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)
+
+ events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.');
+
+ var scriptTypes = getScriptTypesForHook(type);
+ if (scriptTypes == null)
+
+ var scriptFilesToRun = getScriptFiles(pluginElement, scriptTypes, platform);
+ var context =
;
+
+ 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 =
+
+ 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
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)
+
+ 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
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)
+
+ events.emit('debug', 'Executing "' + type + '" hook for "' + plugin_id + '" on ' + platform + '.');
+
+ var scriptTypes = getScriptTypesForHook(type);
+ if (scriptTypes == null)
+
+ var scriptFilesToRun = getScriptFiles(pluginElement, scriptTypes, platform);
+ var context =
;
+
+ 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 =
+
+ 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`.
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:
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.
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
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
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
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
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)
+ 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)
+ 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.
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)
+ 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)
+ 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?
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)
+ 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)
+ 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().
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)
+ 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)
+ 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!
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)
+ 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)
+ 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 all plugins.');
+ return getAllPluginsHookScriptFiles(hook, opts);
+}
+
+/**
+ * Gets application level hooks from the directrory specified.
+ */
+function getApplicationHookScriptsFromDir(dir) {
+ if (!(fs.existsSync(dir)))
+
+ var compareNumbers = function(a, b)
;
+
+ var scripts = fs.readdirSync(dir).sort(compareNumbers).filter(function(s)
);
+ return scripts.map(function (scriptPath) {
+ // for old style hook files we don't use module loader for backward compatibility
+ return
;
+ });
+}
+
+/**
+ * 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
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).
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)
+ 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)
+ 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 all plugins.');
+ return getAllPluginsHookScriptFiles(hook, opts);
+}
+
+/**
+ * Gets application level hooks from the directrory specified.
+ */
+function getApplicationHookScriptsFromDir(dir) {
+ if (!(fs.existsSync(dir)))
+
+ var compareNumbers = function(a, b)
;
+
+ var scripts = fs.readdirSync(dir).sort(compareNumbers).filter(function(s)
);
+ return scripts.map(function (scriptPath) {
+ // for old style hook files we don't use module loader for backward compatibility
+ return
;
+ });
+}
+
+/**
+ * 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
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.
Github user sgrebnov commented on the pull request:
https://github.com/apache/cordova-lib/pull/55#issuecomment-50883550
Addressed @kamrik code review notes
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.
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
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
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?)
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.
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.
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).
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', ...)
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?
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.
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.
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
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
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.
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
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
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
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)
);
var knowOpts =
;
var shortHands =
;
var parsedCmdLine = nopt(knowOpts, shortHands, cmdLine, 0);
console.log(parsedCmdLine);
if(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
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?
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.
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?
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 ..
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
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
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
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 ?
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
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
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
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
Github user sgrebnov commented on the pull request:
https://github.com/apache/cordova-lib/pull/55#issuecomment-57022338
merged, thx @csantanapr!
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)
Github user sgrebnov commented on the pull request:
https://github.com/apache/cordova-plugman/pull/74#issuecomment-40968416
Created issue
CB-6481to 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;
}
```