Skip to content

Commit

Permalink
Remove saving platforms/plugins to config.xml (#750)
Browse files Browse the repository at this point in the history 
We've had one major version of cordova-lib that attempted to ensure both
config.xml and package.json were kept in sync, and now it's time to push
people more strongly towards using package.json.

We will still attempt to read values from config.xml and update them in
package.json, but we will no longer reflect changes back to config.xml.
Whatever's saved in package.json should always have priority over what
is read from config.xml.

The next phase will be to improve the handling of package.json updates,
and then in the next major to completely remove the code that looks at
config.xml for platforms/plugins.
  • Loading branch information
@dpogue @erisu
dpogue authored and erisu committed Mar 15, 2019
1 parent b5edb57 commit 8371d7c
Show file tree
Hide file tree
Showing 11 changed files with 24 additions and 236 deletions.
84 changes: 3 additions & 81 deletions integration-tests/pkgJson.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,13 +105,6 @@ describe('pkgJson', function () {
};
}

function stringContaining (substring) {
return {
asymmetricMatch: s => typeof s === 'string' && s.includes(substring),
jasmineToString: _ => `<stringContaining(${substring})>`
};
}

const customMatchers = {
toSatisfy: () => ({ compare (version, spec) {
const pass = semver.satisfies(version, spec);
Expand Down Expand Up @@ -155,10 +148,8 @@ describe('pkgJson', function () {
// Check that the plugin and spec add was successful to pkg.json.
expect(getPkgJson('cordova.plugins')[pluginId]).toBeDefined();
expect(getPkgJson('dependencies')[pluginId]).tohaveMinSatisfyingVersion('1.1.2');
// Check that the plugin and spec add was successful to config.xml.
expect(getCfg().getPlugins()).toEqual([
{ name: pluginId, spec: specWithMinSatisfyingVersion('1.1.2'), variables: {} }
]);

expect(getCfg().getPluginIdList()).toEqual([]);
}).then(function () {
// And now remove it with --save.
return cordova.plugin('rm', pluginId, { save: true });
Expand Down Expand Up @@ -267,12 +258,6 @@ describe('pkgJson', function () {
return cordova.platform('add', PLATFORM, { save: true })
.then(function () {
expect(getPkgJson('cordova.platforms')).toEqual([PLATFORM]);

// Config.xml and ios/cordova/version check.
expect(getCfg().getEngines()).toEqual([{
name: PLATFORM,
spec: specSatisfiedBy(platformVersion(PLATFORM))
}]);
}).then(function () {
return cordova.plugin('add', PLUGIN, { save: true });
}).then(function () {
Expand All @@ -281,7 +266,6 @@ describe('pkgJson', function () {

// Check that installed version satisfies the dependency spec
const version = pluginVersion(PLUGIN);
expect(version).toSatisfy(getCfg().getPlugin(PLUGIN).spec);
expect(version).toSatisfy(getPkgJson(`dependencies.${PLUGIN}`));
});
});
Expand All @@ -299,25 +283,13 @@ describe('pkgJson', function () {
// Pkg.json has platform
expect(getPkgJson('cordova.platforms')).toEqual([PLATFORM]);
expect(getPkgJson(`dependencies.cordova-${PLATFORM}`)).toBeDefined();

// browser platform and spec have been added to config.xml.
expect(getCfg().getEngines()).toEqual([
{ name: PLATFORM, spec: stringContaining(platformPath) }
]);
}).then(function () {
// Run cordova plugin add local path --save --fetch.
return cordova.plugin('add', pluginPath, { save: true });
}).then(function () {
// Pkg.json has test plugin.
expect(getPkgJson(`cordova.plugins.${PLUGIN}`)).toBeDefined();
expect(getPkgJson(`dependencies.${PLUGIN}`)).toBeDefined();

// Check that plugin and spec have been added to config.xml
expect(getCfg().getPlugins()).toEqual([{
name: PLUGIN,
spec: stringContaining(pluginPath),
variables: {}
}]);
});
});
});
Expand Down Expand Up @@ -409,11 +381,7 @@ describe('pkgJson', function () {
'cordova-browser': specWithMinSatisfyingVersion('5.0.1')
});

// Check that android and browser were added to config.xml with the correct spec.
expect(getCfg().getEngines()).toEqual([
{ name: 'android', spec: '~7.0.0' },
{ name: 'browser', spec: '~5.0.1' }
]);
expect(getCfg().getEngines()).toEqual([]);
}).then(function () {
// And now remove it with --save.
return cordova.platform('rm', ['android', 'browser'], { save: true });
Expand Down Expand Up @@ -456,9 +424,6 @@ describe('pkgJson', function () {
expect(getPkgJson('cordova.platforms')).toEqual([ PLATFORM ]);
// Config.xml and ios/cordova/version check.
const version = platformVersion(PLATFORM);
expect(getCfg().getEngines()).toEqual([
{ name: PLATFORM, spec: specSatisfiedBy(version) }
]);
// Check that pkg.json and ios/cordova/version versions "satisfy" each other.
const pkgSpec = getPkgJson(`dependencies.cordova-${PLATFORM}`);
expect(version).toSatisfy(pkgSpec);
Expand Down Expand Up @@ -491,11 +456,6 @@ describe('pkgJson', function () {
}).then(function () {
// pkg.json has new platform.
expect(getPkgJson('cordova.platforms')).toEqual([PLATFORM]);
// Config.xml and ios/cordova/version check.
expect(getCfg().getEngines()).toEqual([{
name: PLATFORM,
spec: specSatisfiedBy(platformVersion(PLATFORM))
}]);
}).then(function () {
return cordova.plugin('add', PLUGIN, { save: true });
}).then(function () {
Expand Down Expand Up @@ -535,11 +495,6 @@ describe('pkgJson', function () {
return cordova.platform('add', `${PLATFORM}@4.5.4`, { save: true }).then(function () {
// Pkg.json has ios.
expect(getPkgJson('cordova.platforms')).toEqual([ PLATFORM ]);
// Config.xml and ios/cordova/version check.
expect(getCfg().getEngines()).toEqual([{
name: PLATFORM,
spec: specSatisfiedBy(platformVersion(PLATFORM))
}]);
}).then(function () {
return cordova.plugin('add', `${PLUGIN}@4.0.0`, { save: true });
}).then(function () {
Expand All @@ -550,37 +505,4 @@ describe('pkgJson', function () {
});
});
});

// No pkg.json included in test file.
describe('local path is added to config.xml without pkg.json', function () {
beforeEach(() => useProject('basePkgJson13'));

// Test#026: has NO pkg.json. Checks if local path is added to config.xml and has no errors.
it('Test#026 : if you add a platform with local path, config.xml gets updated', function () {
const PLATFORM = 'browser';
const platformPath = copyFixture(`platforms/cordova-${PLATFORM}`);

return cordova.platform('add', platformPath, { save: true })
.then(function () {
expect(getCfg().getEngines()).toContain({
name: 'browser', spec: stringContaining(platformPath)
});
});
});

// Test#027: has NO pkg.json. Checks if local path is added to config.xml and has no errors.
it('Test#027 : if you add a plugin with local path, config.xml gets updated', function () {
const PLUGIN = 'cordova-lib-test-plugin';
const pluginPath = copyFixture(path.join('plugins', PLUGIN));

return cordova.plugin('add', pluginPath, { save: true })
.then(function () {
expect(getCfg().getPlugins()).toContain({
name: PLUGIN,
spec: stringContaining(pluginPath),
variables: {}
});
});
});
});
});
8 changes: 0 additions & 8 deletions spec/cordova/platform/addHelper.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,14 +217,6 @@ describe('cordova/platform/addHelper', function () {
});
});

it('should write out the version of platform just added/updated to config.xml if the save option is provided', function () {
return platform_addHelper('add', hooks_mock, projectRoot, ['ios'], { save: true, restoring: true }).then(function (result) {
expect(cfg_parser_mock.prototype.removeEngine).toHaveBeenCalled();
expect(cfg_parser_mock.prototype.addEngine).toHaveBeenCalled();
expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
});
});

describe('if the project contains a package.json', function () {
it('should write out the platform just added/updated to the cordova.platforms property of package.json', function () {
fs.readFileSync.and.returnValue('file');
Expand Down
3 changes: 2 additions & 1 deletion spec/cordova/platform/remove.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ describe('cordova/platform/remove', function () {
beforeEach(function () {
hooks_mock = jasmine.createSpyObj('hooksRunner mock', ['fire']);
hooks_mock.fire.and.returnValue(Promise.resolve());
cfg_parser_mock.prototype = jasmine.createSpyObj('config parser mock', ['write', 'removeEngine']);
cfg_parser_mock.prototype = jasmine.createSpyObj('config parser mock', ['write', 'getEngines', 'removeEngine']);

platform_remove = rewire('../../../src/cordova/platform/remove');
platform_remove.__set__({
Expand All @@ -50,6 +50,7 @@ describe('cordova/platform/remove', function () {
spyOn(cordova_util, 'removePlatformPluginsJson');
spyOn(events, 'emit');
spyOn(cordova_util, 'requireNoCache').and.returnValue({});
cfg_parser_mock.prototype.getEngines.and.returnValue([{ name: 'atari', spec: '^0.0.1' }]);
});

describe('error/warning conditions', function () {
Expand Down
5 changes: 1 addition & 4 deletions spec/cordova/plugin/add.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ describe('cordova/plugin/add', function () {
beforeEach(function () {
hook_mock = jasmine.createSpyObj('hooks runner mock', ['fire']);
hook_mock.fire.and.returnValue(Promise.resolve());
Cfg_parser_mock.prototype = jasmine.createSpyObj('config parser prototype mock', ['getPlugin', 'removePlugin', 'addPlugin', 'write']);
Cfg_parser_mock.prototype = jasmine.createSpyObj('config parser prototype mock', ['getPlugin']);
cfg_parser_revert_mock = add.__set__('ConfigParser', Cfg_parser_mock);
plugin_info = jasmine.createSpyObj('pluginInfo', ['getPreferences']);
plugin_info.getPreferences.and.returnValue({});
Expand Down Expand Up @@ -164,9 +164,6 @@ describe('cordova/plugin/add', function () {
expect(add.determinePluginTarget).toHaveBeenCalledWith(jasmine.anything(), jasmine.anything(), jasmine.anything(), jasmine.objectContaining({ 'variables': cli_plugin_variables }));
// check that the plugin variables from config.xml got added to cli_variables
expect(plugman.install).toHaveBeenCalledWith(jasmine.anything(), jasmine.anything(), jasmine.anything(), jasmine.anything(), jasmine.objectContaining({ 'cli_variables': cli_plugin_variables }));
expect(Cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('cordova-plugin-device');
expect(Cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(jasmine.any(Object), cli_plugin_variables);
expect(Cfg_parser_mock.prototype.write).toHaveBeenCalled();
});
});
// can't test the following due to inline require of preparePlatforms
Expand Down
3 changes: 2 additions & 1 deletion spec/cordova/plugin/remove.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ describe('cordova/plugin/remove', function () {
hook_mock = jasmine.createSpyObj('hooks runner mock', ['fire']);
spyOn(prepare, 'preparePlatforms').and.returnValue(true);
hook_mock.fire.and.returnValue(Promise.resolve());
cfg_parser_mock.prototype = jasmine.createSpyObj('config parser mock', ['write', 'removeEngine', 'addEngine', 'getHookScripts', 'removePlugin']);
cfg_parser_mock.prototype = jasmine.createSpyObj('config parser mock', ['write', 'getPlugin', 'removePlugin']);
cfg_parser_revert_mock = remove.__set__('ConfigParser', cfg_parser_mock);
plugin_info_provider_mock.prototype = jasmine.createSpyObj('plugin info provider mock', ['get', 'getPreferences']);
plugin_info_provider_mock.prototype.get = function (directory) {
Expand Down Expand Up @@ -159,6 +159,7 @@ describe('cordova/plugin/remove', function () {
fs.existsSync.and.returnValue(true);
remove.validatePluginId.and.returnValue('cordova-plugin-splashscreen');
var opts = { important: 'options', plugins: ['cordova-plugin-splashscreen'] };
cfg_parser_mock.prototype.getPlugin.and.returnValue({});
return remove(projectRoot, 'cordova-plugin-splashscreen', hook_mock, opts).then(function () {
expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalled();
expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
Expand Down
80 changes: 5 additions & 75 deletions spec/cordova/restore-util.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,6 @@ describe('cordova/restore-util', () => {
}

function expectConsistentPlugins (plugins) {
expect(getCfg().getPlugins()).toEqual(jasmine.arrayWithExactContents(plugins));

const unwrappedPlugins = plugins.map(p => p.sample || p);
expectPluginsInPkgJson(unwrappedPlugins);

Expand All @@ -160,31 +158,6 @@ describe('cordova/restore-util', () => {

describe('installPlatformsFromConfigXML', () => {

it('Test#000 : should change specs in config.xml from using ~ to using ^', () => {
const PLATFORM = 'android';

getCfg()
.addEngine(PLATFORM, '~7.1.1')
.write();

setPkgJson('dependencies', {
[platformPkgName(PLATFORM)]: '^7.1.1'
});

return restore.installPlatformsFromConfigXML().then(() => {
// No changes to pkg.json spec for android.
const pkgJson = getPkgJson();
expect(pkgJson.cordova.platforms).toEqual([PLATFORM]);
expect(pkgJson.dependencies[platformPkgName(PLATFORM)]).toMatch(/^\^/);

// config.xml spec should change from '~' to '^'.
expect(getCfg().getEngines()).toEqual([{
name: PLATFORM,
spec: jasmine.stringMatching(/^\^/)
}]);
});
});

it('Test#001 : should restore saved platform from package.json', () => {
setPkgJson('cordova.platforms', [testPlatform]);

Expand All @@ -205,7 +178,7 @@ describe('cordova/restore-util', () => {

return restore.installPlatformsFromConfigXML().then(() => {
// Check config.xml for spec modification.
expect(getCfg().getEngines()).toEqual([{
expect(getCfg().getEngines()).not.toEqual([{
name: PLATFORM,
spec: `git+${PLATFORM_URL}.git`
}]);
Expand Down Expand Up @@ -269,28 +242,6 @@ describe('cordova/restore-util', () => {
});
});

it('Test#007 : should update config.xml to include platforms from package.json', () => {
getCfg()
.addEngine('ios', '6.0.0')
.write();
setPkgJson('dependencies', {
'cordova-ios': '^4.5.4', 'cordova-browser': '^5.0.3'
});
setPkgJson('cordova.platforms', ['ios', 'browser']);

return restore.installPlatformsFromConfigXML().then(() => {
// Check to make sure that 'browser' spec was added properly.
expect(getCfg().getEngines()).toEqual([
{ name: 'ios', spec: '^4.5.4' },
{ name: 'browser', spec: '^5.0.3' }
]);
// No change to pkg.json dependencies.
expect(getPkgJson('dependencies')).toEqual({
'cordova-ios': '^4.5.4', 'cordova-browser': '^5.0.3'
});
});
});

it('Test#016 : should restore platforms & plugins and create a missing package.json', () => {
getCfg()
.addEngine(testPlatform)
Expand Down Expand Up @@ -392,7 +343,7 @@ describe('cordova/restore-util', () => {
});

return restore.installPluginsFromConfigXML({ save: true }).then(() => {
expectConsistentPlugins([
expectPluginsInPkgJson([
jasmine.objectContaining({
name: 'cordova-plugin-camera',
variables: { variable_1: ' ', variable_2: ' ', variable_3: 'value_3' }
Expand All @@ -405,7 +356,7 @@ describe('cordova/restore-util', () => {
name: 'cordova-plugin-device',
variables: { variable_1: 'value_1' }
})
]);
].map(p => p.sample || p));
});
});

Expand Down Expand Up @@ -436,7 +387,7 @@ describe('cordova/restore-util', () => {
});

return restore.installPluginsFromConfigXML({ save: true }).then(() => {
expectConsistentPlugins([{
expectPluginsInPkgJson([{
name: 'cordova-plugin-camera',
spec: '^2.3.0',
variables: { variable_1: 'value_1', variable_2: 'value_2', variable_3: 'value_3' }
Expand All @@ -448,28 +399,7 @@ describe('cordova/restore-util', () => {
name: 'cordova-plugin-splashscreen',
spec: undefined,
variables: {}
}]);
});
});

/**
* When config has no plugins and is restored, the plugins will be restored with the
* pkg.json plugins and with the spec from pkg.json dependencies.
*/
it('Test#015 : update config.xml to include all plugins/variables from pkg.json', () => {
setPkgJson('dependencies', {
'cordova-plugin-camera': '^2.3.0'
});
setPkgJson('cordova.plugins', {
'cordova-plugin-camera': { variable_1: 'value_1' }
});

return restore.installPluginsFromConfigXML({ save: true }).then(() => {
expectConsistentPlugins([{
name: 'cordova-plugin-camera',
spec: '^2.3.0',
variables: { variable_1: 'value_1' }
}]);
}].map(p => p.sample || p));
});
});

Expand Down
7 changes: 0 additions & 7 deletions src/cordova/platform/addHelper.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,13 +220,6 @@ function addHelper (cmd, hooksRunner, projectRoot, targets, opts) {
// was installed. However, we save it with the "~" attribute (this allows for patch updates).
spec = saveVersion ? '~' + platDetails.version : spec;

// Save target into config.xml, overriding already existing settings.
events.emit('log', '--save flag or autosave detected');
events.emit('log', 'Saving ' + platform + '@' + spec + ' into config.xml file ...');
cfg.removeEngine(platform);
cfg.addEngine(platform, spec);
cfg.write();

// Save to add to pacakge.json's cordova.platforms array in the next then.
platformsToSave.push(platform);
}
Expand Down
Loading

0 comments on commit 8371d7c

Please sign in to comment.