Skip to content
Permalink
Browse files
Break dependency cycles (#807)
* Break cycles caused by require-facade anti-pattern

All these dependency cycles were caused by sub-package members
requiring their respective sub-package facade module when all they
needed was another member from their own sub-package.

Cycles that were broken by this:
- cordova/{cordova > build}
- cordova/{cordova > emulate}
- cordova/{cordova > run}
- cordova/{cordova > serve}
- cordova/platform/{index > addHelper}
- plugman/{plugman > install}

* Break cycle between cordova/util and platforms/platforms

We do this by moving `cordova/util.hostSupports` to the `platforms`
sub-package. To not further pollute `platforms/platforms`, we introduce
a new facade module for the `platforms` sub-package.

`platforms/platforms` remains for the time being, especially because it
is part of our public interface (see `cordova-lib.js`).

This commit also simplifies the platforms.hostSupports utility function.
The support for indicating no host requirements by setting `hostos: '*'`
is dropped in favor of letting `hostos` undefined.

* Break cycles involving cordova/prepare.preparePlatforms

To achieve this, preparePlatforms had to be extracted into its own
module, so it could be required separately from cordova/prepare.
It now lives in cordova/prepare/platforms.

Cycles that were broken by this:

- cordova/{prepare > restore-util > ... > platform/addHelper}
- cordova/{prepare > restore-util > ... > plugin/add}
- cordova/{prepare > restore-util > ... > plugin/remove}

* Re-enable tests that needed to spy on preparePlatforms

These tests were disabled because it was difficult to spy on
preparePlatforms since it was only required during runtime.

The test in cordova/plugin/add.spec wasn't even implemented. I did my
best to follow the tests description with my implementation.
  • Loading branch information
raphinesse committed Oct 21, 2019
1 parent 270c3ca commit 545fb910e4f8f704ac2f5b370473aec2c89eec23
Showing 24 changed files with 387 additions and 284 deletions.
@@ -16,32 +16,30 @@
specific language governing permissions and limitations
under the License.
*/
var cordova = require('../../src/cordova/cordova');
const rewire = require('rewire');
var platforms = require('../../src/platforms/platforms');
var HooksRunner = require('../../src/hooks/HooksRunner');
var util = require('../../src/cordova/util');

var supported_platforms = Object.keys(platforms);

describe('build command', function () {
var is_cordova;
var list_platforms;
var fire;
var project_dir = '/some/path';
var prepare_spy, compile_spy;
let cordovaBuild, cordovaPrepare, cordovaCompile;

beforeEach(function () {
is_cordova = spyOn(util, 'isCordova').and.returnValue(project_dir);
spyOn(util, 'isCordova').and.returnValue(project_dir);
spyOn(util, 'cdProjectRoot').and.returnValue(project_dir);
list_platforms = spyOn(util, 'listPlatforms').and.returnValue(supported_platforms);
fire = spyOn(HooksRunner.prototype, 'fire').and.returnValue(Promise.resolve());
prepare_spy = spyOn(cordova, 'prepare').and.returnValue(Promise.resolve());
compile_spy = spyOn(cordova, 'compile').and.returnValue(Promise.resolve());
spyOn(util, 'listPlatforms').and.returnValue(Object.keys(platforms));
spyOn(HooksRunner.prototype, 'fire').and.returnValue(Promise.resolve());

cordovaBuild = rewire('../../src/cordova/build');
cordovaPrepare = jasmine.createSpy('cordovaPrepare').and.returnValue(Promise.resolve());
cordovaCompile = jasmine.createSpy('cordovaCompile').and.returnValue(Promise.resolve());
cordovaBuild.__set__({ cordovaPrepare, cordovaCompile });
});
describe('failure', function () {
it('Test 001 : should not run inside a project with no platforms', function () {
list_platforms.and.returnValue([]);
return cordova.build()
util.listPlatforms.and.returnValue([]);
return cordovaBuild()
.then(function () {
fail('Expected promise to be rejected');
}, function (err) {
@@ -53,9 +51,9 @@ describe('build command', function () {
});

it('Test 002 : should not run outside of a Cordova-based project', function () {
is_cordova.and.returnValue(false);
util.isCordova.and.returnValue(false);

return cordova.build()
return cordovaBuild()
.then(function () {
fail('Expected promise to be rejected');
}, function (err) {
@@ -69,39 +67,41 @@ describe('build command', function () {

describe('success', function () {
it('Test 003 : should run inside a Cordova-based project with at least one added platform and call both prepare and compile', function () {
return cordova.build(['android', 'ios']).then(function () {
return cordovaBuild(['android', 'ios']).then(function () {
var opts = Object({ platforms: [ 'android', 'ios' ], verbose: false, options: Object({ }) });
expect(prepare_spy).toHaveBeenCalledWith(opts);
expect(compile_spy).toHaveBeenCalledWith(opts);
expect(cordovaPrepare).toHaveBeenCalledWith(opts);
expect(cordovaCompile).toHaveBeenCalledWith(opts);
});
});
it('Test 004 : should pass down options', function () {
return cordova.build({ platforms: ['android'], options: { release: true } }).then(function () {
return cordovaBuild({ platforms: ['android'], options: { release: true } }).then(function () {
var opts = { platforms: ['android'], options: { release: true }, verbose: false };
expect(prepare_spy).toHaveBeenCalledWith(opts);
expect(compile_spy).toHaveBeenCalledWith(opts);
expect(cordovaPrepare).toHaveBeenCalledWith(opts);
expect(cordovaCompile).toHaveBeenCalledWith(opts);
});
});
});

describe('hooks', function () {
describe('when platforms are added', function () {
it('Test 006 : should fire before hooks through the hooker module', function () {
return cordova.build(['android', 'ios']).then(function () {
expect(fire.calls.argsFor(0)).toEqual(['before_build', { verbose: false, platforms: ['android', 'ios'], options: {} }]);
return cordovaBuild(['android', 'ios']).then(function () {
expect(HooksRunner.prototype.fire.calls.argsFor(0))
.toEqual(['before_build', { verbose: false, platforms: ['android', 'ios'], options: {} }]);
});
});
it('Test 007 : should fire after hooks through the hooker module', function () {
return cordova.build('android').then(function () {
expect(fire.calls.argsFor(1)).toEqual([ 'after_build', { platforms: [ 'android' ], verbose: false, options: {} } ]);
return cordovaBuild('android').then(function () {
expect(HooksRunner.prototype.fire.calls.argsFor(1))
.toEqual([ 'after_build', { platforms: [ 'android' ], verbose: false, options: {} } ]);
});
});
});

describe('with no platforms added', function () {
it('Test 008 : should not fire the hooker', function () {
list_platforms.and.returnValue([]);
return Promise.resolve().then(cordova.build).then(function () {
util.listPlatforms.and.returnValue([]);
return Promise.resolve().then(cordovaBuild).then(function () {
fail('Expected promise to be rejected');
}, function (err) {
expect(err).toEqual(jasmine.any(Error));
@@ -16,37 +16,37 @@
specific language governing permissions and limitations
under the License.
*/
var cordova = require('../../src/cordova/cordova');
const rewire = require('rewire');
var platforms = require('../../src/platforms/platforms');
var HooksRunner = require('../../src/hooks/HooksRunner');
var util = require('../../src/cordova/util');

var supported_platforms = Object.keys(platforms);

describe('emulate command', function () {
var is_cordova;
var list_platforms;
var fire;
var project_dir = '/some/path';
var prepare_spy, platformApi, getPlatformApi;
let cordovaEmulate, cordovaPrepare, platformApi, getPlatformApi;

beforeEach(function () {
is_cordova = spyOn(util, 'isCordova').and.returnValue(project_dir);
spyOn(util, 'isCordova').and.returnValue(project_dir);
spyOn(util, 'cdProjectRoot').and.returnValue(project_dir);
list_platforms = spyOn(util, 'listPlatforms').and.returnValue(supported_platforms);
fire = spyOn(HooksRunner.prototype, 'fire').and.returnValue(Promise.resolve());
prepare_spy = spyOn(cordova, 'prepare').and.returnValue(Promise.resolve());
spyOn(util, 'listPlatforms').and.returnValue(supported_platforms);
spyOn(HooksRunner.prototype, 'fire').and.returnValue(Promise.resolve());

cordovaEmulate = rewire('../../src/cordova/emulate');
cordovaPrepare = jasmine.createSpy('cordovaPrepare').and.returnValue(Promise.resolve());
cordovaEmulate.__set__({ cordovaPrepare });

platformApi = {
run: jasmine.createSpy('run').and.returnValue(Promise.resolve()),
build: jasmine.createSpy('build').and.returnValue(Promise.resolve())
};

getPlatformApi = spyOn(platforms, 'getPlatformApi').and.returnValue(platformApi);
});
describe('failure', function () {
it('Test 001 : should not run inside a Cordova-based project with no added platforms by calling util.listPlatforms', function () {
list_platforms.and.returnValue([]);
return cordova.emulate()
util.listPlatforms.and.returnValue([]);
return cordovaEmulate()
.then(function () {
fail('Expected promise to be rejected');
}, function (err) {
@@ -55,8 +55,8 @@ describe('emulate command', function () {
});
});
it('Test 002 : should not run outside of a Cordova-based project', function () {
is_cordova.and.returnValue(false);
return cordova.emulate()
util.isCordova.and.returnValue(false);
return cordovaEmulate()
.then(function () {
fail('Expected promise to be rejected');
}, function (err) {
@@ -67,64 +67,58 @@ describe('emulate command', function () {

describe('success', function () {
it('Test 003 : should run inside a Cordova-based project with at least one added platform and call prepare and shell out to the emulate script', function () {
return cordova.emulate(['android', 'ios'])
return cordovaEmulate(['android', 'ios'])
.then(function () {
expect(prepare_spy).toHaveBeenCalledWith(jasmine.objectContaining({ platforms: ['android', 'ios'] }));
expect(cordovaPrepare).toHaveBeenCalledWith(jasmine.objectContaining({ platforms: ['android', 'ios'] }));
expect(getPlatformApi).toHaveBeenCalledWith('android');
expect(getPlatformApi).toHaveBeenCalledWith('ios');
expect(platformApi.build).toHaveBeenCalled();
expect(platformApi.run).toHaveBeenCalled();
});
});
it('Test 004 : should pass down options', function () {
return cordova.emulate({ platforms: ['ios'], options: { optionTastic: true } })
return cordovaEmulate({ platforms: ['ios'], options: { optionTastic: true } })
.then(function () {
expect(prepare_spy).toHaveBeenCalledWith(jasmine.objectContaining({ platforms: ['ios'] }));
expect(cordovaPrepare).toHaveBeenCalledWith(jasmine.objectContaining({ platforms: ['ios'] }));
expect(getPlatformApi).toHaveBeenCalledWith('ios');
expect(platformApi.build).toHaveBeenCalledWith({ device: false, emulator: true, optionTastic: true });
expect(platformApi.run).toHaveBeenCalledWith({ device: false, emulator: true, optionTastic: true, nobuild: true });
});
});

describe('run parameters should not be altered by intermediate build command', function () {
var originalBuildSpy;
beforeEach(function () {
originalBuildSpy = platformApi.build;
platformApi.build = jasmine.createSpy('build').and.callFake(function (opts) {
platformApi.build.and.callFake(opts => {
opts.couldBeModified = 'insideBuild';
return Promise.resolve();
});
});
afterEach(function () {
platformApi.build = originalBuildSpy;
});
it('Test 006 : should leave parameters unchanged', function () {
const baseOptions = { password: '1q1q', device: false, emulator: true };
const expectedRunOptions = Object.assign({ nobuild: true }, baseOptions);
const expectedBuildOptions = Object.assign({ couldBeModified: 'insideBuild' }, baseOptions);

return cordova.emulate({ platforms: ['blackberry10'], options: { password: '1q1q' } })
return cordovaEmulate({ platforms: ['blackberry10'], options: { password: '1q1q' } })
.then(function () {
expect(prepare_spy).toHaveBeenCalledWith({ platforms: [ 'blackberry10' ], options: expectedBuildOptions, verbose: false });
expect(cordovaPrepare).toHaveBeenCalledWith({ platforms: [ 'blackberry10' ], options: expectedBuildOptions, verbose: false });
expect(platformApi.build).toHaveBeenCalledWith(expectedBuildOptions);
expect(platformApi.run).toHaveBeenCalledWith(expectedRunOptions);
});
});
});

it('Test 007 : should call platform\'s build method', function () {
return cordova.emulate({ platforms: ['blackberry10'] })
return cordovaEmulate({ platforms: ['blackberry10'] })
.then(function () {
expect(prepare_spy).toHaveBeenCalled();
expect(cordovaPrepare).toHaveBeenCalled();
expect(platformApi.build).toHaveBeenCalledWith({ device: false, emulator: true });
expect(platformApi.run).toHaveBeenCalledWith(jasmine.objectContaining({ nobuild: true }));
});
});

it('Test 008 : should not call build if --nobuild option is passed', function () {
return cordova.emulate({ platforms: ['blackberry10'], options: { nobuild: true } })
return cordovaEmulate({ platforms: ['blackberry10'], options: { nobuild: true } })
.then(function () {
expect(prepare_spy).toHaveBeenCalled();
expect(cordovaPrepare).toHaveBeenCalled();
expect(platformApi.build).not.toHaveBeenCalled();
expect(platformApi.run).toHaveBeenCalledWith(jasmine.objectContaining({ nobuild: true }));
});
@@ -134,30 +128,30 @@ describe('emulate command', function () {
describe('hooks', function () {
describe('when platforms are added', function () {
it('Test 009 : should fire before hooks through the hooker module', function () {
return cordova.emulate(['android', 'ios'])
return cordovaEmulate(['android', 'ios'])
.then(function () {
expect(fire).toHaveBeenCalledWith('before_emulate',
expect(HooksRunner.prototype.fire).toHaveBeenCalledWith('before_emulate',
jasmine.objectContaining({ verbose: false, platforms: ['android', 'ios'], options: jasmine.any(Object) }));
});
});
it('Test 010 : should fire after hooks through the hooker module', function () {
return cordova.emulate('android')
return cordovaEmulate('android')
.then(function () {
expect(fire).toHaveBeenCalledWith('after_emulate',
expect(HooksRunner.prototype.fire).toHaveBeenCalledWith('after_emulate',
jasmine.objectContaining({ verbose: false, platforms: ['android'], options: jasmine.any(Object) }));
});
});
});

describe('with no platforms added', function () {
it('Test 011 : should not fire the hooker', function () {
list_platforms.and.returnValue([]);
return Promise.resolve().then(cordova.emulate).then(function () {
util.listPlatforms.and.returnValue([]);
return Promise.resolve().then(cordovaEmulate).then(function () {
fail('Expected promise to be rejected');
}, function (err) {
expect(err).toEqual(jasmine.any(Error));
expect(err.message).toContain('No platforms added to this project. Please use `cordova platform add <platform>`.');
expect(fire).not.toHaveBeenCalled();
expect(HooksRunner.prototype.fire).not.toHaveBeenCalled();
});
});
});

0 comments on commit 545fb91

Please sign in to comment.