Limit requireCordovaModule to Cordova modules

[raphinesse]

The Context

The context provided to hooks has a method called requireCordovaModule.

cordova-lib/src/hooks/Context.js

Lines 50 to 72 in 1bc9dd0

	/**
	* Returns a required module
	* @param {String} modulePath Module path
	* @returns {Object} */
	Context.prototype.requireCordovaModule = function (modulePath) {
	// There is a very common mistake, when hook requires some cordova functionality
	// using 'cordova-lib/...' path.
	// This path will be resolved only when running cordova from 'normal' installation
	// (without symlinked modules). If cordova-lib linked to cordova-cli this path is
	// never resolved, so hook fails with 'Error: Cannot find module 'cordova-lib''
	var resolvedPath = path.resolve(__dirname, modulePath.replace(/^cordova-lib/, '../../../cordova-lib'));
	var relativePath = path.relative(__dirname, resolvedPath).replace(/\\/g, '/');
	events.emit('verbose', 'Resolving module name for ' + modulePath + ' => ' + relativePath);
	var compatRequire = compatMap[relativePath];
	if (compatRequire) {
	events.emit('warn', 'The module "' + path.basename(relativePath) + '" has been factored ' +
	'into "cordova-common". Consider update your plugin hooks.');
	return compatRequire();
	}
	return require(relativePath);
	};

The apparent purpose of this method is to allow users to require modules of the exact version of Cordova that is running the hook. I guess that makes some sense, given how we currently use the Node module system to provide singletons.

The Problem

However, this method conceptually leaks all internals of cordova-lib, including any of its dependencies, to the hooks context. In fact, the only documentation on this method even encourages users to use it to require Q, which we could break, if we removed Q from our dependencies as part of #681. I can see how this might have seemed like a great idea when promises weren't available in native JS, but that has thankfully changed now.

The Solution

I suggest we limit the modules allowed to import using Context.prototype.requireCordovaModule to a fixed whitelist containing only cordova-* dependencies of cordova-lib and cordova-lib itself.

As a first step, we should only issue a deprecation warning if anything not on the whitelist is required.

As an additional safety measure, we might warn users about using private interfaces when they deep-require something (like require('cordova-lib/src/hooks/Context') for example).

What do you think? I'm looking forward to your feedback.

[raphinesse]

After #706, I'm beginning to think we should consider rolling out the deprecation step even earlier than the next major.

[brodycj]

I would personally favor starting the deprecation now and limiting requireCordovaModule to Cordova modules in the next major.

[brodycj]

Shouldn't we consider dropping support for requireCordovaModule('cordova-lib') or requireCordovaModule('*/cordova-lib') altogether?

[raphinesse]

Shouldn't we consider dropping support for requireCordovaModule('cordova-lib') or requireCordovaModule('*/cordova-lib') altogether?

I'm not sure what you mean by requireCordovaModule('*/cordova-lib'). After #709, The argument for requireCordovaModule must be either the name of a Cordova package (cordova-*) or a path to a module in a Cordova package (cordova-*/**).

In case you meant we should drop the method altogether: I actually considered that. But as long as the predominant usage of cordova-lib & Co. is through a globally installed package cordova I think we need a means for people to require modules from the currently executing version of Cordova. Which is exactly what requireCordovaModule provides.

---

That being said, I would prefer that people require cordova-lib or cordova as a dependency in their projects and use only that to build them. That way regular require would pick up the right versions of everything and it would be perfectly clear which version of Cordova is required to build a project.