You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by surajpindoria <gi...@git.apache.org> on 2014/07/29 20:53:16 UTC
[GitHub] cordova-lib pull request: CB-7190: Add browserify support in cordo...
GitHub user surajpindoria opened a pull request:
https://github.com/apache/cordova-lib/pull/66
CB-7190: Add browserify support in cordova-lib/cordova-cli
https://issues.apache.org/jira/browse/CB-7190
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/surajpindoria/cordova-lib browserify
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-lib/pull/66.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 #66
----
commit 6e477343f454f2e5cdad0b00f4148a23466516ce
Author: Suraj Pindoria <su...@yahoo.com>
Date: 2014-07-29T18:51:31Z
CB-7190: Add browserify support in cordova-lib/cordova-cli
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cordova-lib pull request: CB-7190: Add browserify support in cordo...
Posted by stevengill <gi...@git.apache.org>.
Github user stevengill commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/66#discussion_r15553915
--- Diff: cordova-lib/src/plugman/uninstall.js ---
@@ -316,6 +316,10 @@ function handleUninstall(actions, platform, plugin_id, plugin_et, project_dir, w
// queue up the plugin so prepare can remove the config changes
config_changes.add_uninstalled_plugin_to_prepare_queue(plugins_dir, plugin_id, platform, is_top_level);
// call prepare after a successful uninstall
- plugman.prepare(project_dir, platform, plugins_dir, www_dir, is_top_level);
+ if (options.browserify) {
--- End diff --
I agree. It is better design. https://issues.apache.org/jira/browse/CB-7227
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cordova-lib pull request: CB-7190: Add browserify support in cordo...
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/66#discussion_r15544876
--- Diff: cordova-lib/src/plugman/plugman.js ---
@@ -70,6 +70,7 @@ addProperty(plugman, 'install', './install', true);
addProperty(plugman, 'uninstall', './uninstall', true);
addProperty(plugman, 'fetch', './fetch', true);
addProperty(plugman, 'prepare', './prepare');
+addProperty(plugman, 'prepareBrowserify', './prepare-browserify');
--- End diff --
Isn't this already taken care of here:
https://github.com/surajpindoria/cordova-lib/blob/browserify/cordova-lib/src/plugman/plugman.js#L131
the browserify and non-browserify methods have the exact same signature, so we should be calling a generic 'prepare' method which could be either one.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cordova-lib pull request: CB-7190: Add browserify support in cordo...
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/66#discussion_r15553576
--- Diff: cordova-lib/src/plugman/uninstall.js ---
@@ -316,6 +316,10 @@ function handleUninstall(actions, platform, plugin_id, plugin_et, project_dir, w
// queue up the plugin so prepare can remove the config changes
config_changes.add_uninstalled_plugin_to_prepare_queue(plugins_dir, plugin_id, platform, is_top_level);
// call prepare after a successful uninstall
- plugman.prepare(project_dir, platform, plugins_dir, www_dir, is_top_level);
+ if (options.browserify) {
--- End diff --
getting cordova plugin add/rm to run plugman should be the priority I think. I don't see the issue with exploring/doing it ...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cordova-lib pull request: CB-7190: Add browserify support in cordo...
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/66#discussion_r15545141
--- Diff: cordova-lib/src/plugman/uninstall.js ---
@@ -316,6 +316,10 @@ function handleUninstall(actions, platform, plugin_id, plugin_et, project_dir, w
// queue up the plugin so prepare can remove the config changes
config_changes.add_uninstalled_plugin_to_prepare_queue(plugins_dir, plugin_id, platform, is_top_level);
// call prepare after a successful uninstall
- plugman.prepare(project_dir, platform, plugins_dir, www_dir, is_top_level);
+ if (options.browserify) {
--- End diff --
again, all of this switching is not required, nor passing the options.browserify property around because plugman.prepare is already dynamic based on the options.
ie.
IF (options.browserify) is TRUE
THEN so is ( plugman.prepare === plugman.prepareBrowserify )
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cordova-lib pull request: CB-7190: Add browserify support in cordo...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-lib/pull/66
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cordova-lib pull request: CB-7190: Add browserify support in cordo...
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/66#discussion_r15545303
--- Diff: cordova-lib/src/plugman/install.js ---
@@ -571,7 +571,11 @@ function handleInstall(actions, pluginInfo, platform, project_dir, plugins_dir,
// queue up the plugin so prepare knows what to do.
config_changes.add_installed_plugin_to_prepare_queue(plugins_dir, pluginInfo.id, platform, filtered_variables, options.is_top_level);
// call prepare after a successful install
- plugman.prepare(project_dir, platform, plugins_dir, options.www_dir, options.is_top_level);
+ if (options.browserify) {
--- End diff --
conditional check not required here ...
See: https://github.com/apache/cordova-lib/pull/66#discussion_r15545141
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cordova-lib pull request: CB-7190: Add browserify support in cordo...
Posted by stevengill <gi...@git.apache.org>.
Github user stevengill commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/66#discussion_r15553301
--- Diff: cordova-lib/src/plugman/uninstall.js ---
@@ -316,6 +316,10 @@ function handleUninstall(actions, platform, plugin_id, plugin_et, project_dir, w
// queue up the plugin so prepare can remove the config changes
config_changes.add_uninstalled_plugin_to_prepare_queue(plugins_dir, plugin_id, platform, is_top_level);
// call prepare after a successful uninstall
- plugman.prepare(project_dir, platform, plugins_dir, www_dir, is_top_level);
+ if (options.browserify) {
--- End diff --
We added the browserify checks to install & uninstall because ```cordova plugin add/rm``` directly calls ```plugman.raw.install/uninstall```. It skips going through ```plugman.js``` and therefore https://github.com/surajpindoria/cordova-lib/blob/browserify/cordova-lib/src/plugman/plugman.js#L131 never runs.
A better fix then adding these multiple checks would be to get ```cordova plugin add/rm``` to run ```plugman.js```. We haven't explored this yet though.
Once ```prepare-browserify``` becomes the default way of doing things, we can remove the old prepare and remove these checks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cordova-lib pull request: CB-7190: Add browserify support in cordo...
Posted by stevengill <gi...@git.apache.org>.
Github user stevengill commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/66#discussion_r15553653
--- Diff: cordova-lib/src/plugman/plugman.js ---
@@ -70,6 +70,7 @@ addProperty(plugman, 'install', './install', true);
addProperty(plugman, 'uninstall', './uninstall', true);
addProperty(plugman, 'fetch', './fetch', true);
addProperty(plugman, 'prepare', './prepare');
+addProperty(plugman, 'prepareBrowserify', './prepare-browserify');
--- End diff --
This was added so we could spy on prepareBrowserify in the tests the same way we spied on prepare. The install/uninstall issue described below prompted this for us. We may be able to remove it
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---