Skip to content

Commit eb4934b

Browse files
imhoffdimhoffd
committed
fix(cordova): respect --nosave for platform/plugin add
fixes #3524
1 parent e6a5bff commit eb4934b

File tree

10 files changed

+96
-158
lines changed

10 files changed

+96
-158
lines changed

‎packages/@ionic/cli-utils/src/lib/integrations/cordova/__tests__/project.ts

Lines changed: 0 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -21,38 +21,6 @@ describe('@ionic/cli-utils', () => {
2121

2222
});
2323

24-
describe('installPlatform', () => {
25-
26-
const mockRun = jest.fn();
27-
const mockRunWithError = jest.fn(() => {
28-
throw new Error('Platform fakePlatform already added');
29-
});
30-
const env = jest.fn(run => {
31-
return {
32-
shell: { run },
33-
log: {
34-
warn: jest.fn(),
35-
},
36-
};
37-
});
38-
39-
it('should run the process in the cwd', async () => {
40-
await project.installPlatform(new env(mockRun), 'fakePlatform', 'fakeCwd');
41-
try {
42-
await project.installPlatform(new env(mockRunWithError), 'fakePlatform', 'fakeCwd');
43-
} catch (e) {
44-
// ignore
45-
}
46-
expect(mockRun.mock.calls[0][2]).toEqual(jasmine.objectContaining({
47-
cwd: 'fakeCwd',
48-
}));
49-
expect(mockRunWithError.mock.calls[1][2]).toEqual(jasmine.objectContaining({
50-
cwd: 'fakeCwd',
51-
}));
52-
});
53-
54-
});
55-
5624
});
5725

5826
});

‎packages/@ionic/cli-utils/src/lib/integrations/cordova/__tests__/utils.ts

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { filterArgumentsForCordova, generateOptionsForCordovaBuild } from '../ut
22

33
describe('@ionic/cli-utils', () => {
44

5-
const metadata = {
5+
const buildMetadata = {
66
name: 'build',
77
inputs: [
88
{
@@ -37,38 +37,63 @@ describe('@ionic/cli-utils', () => {
3737
]
3838
};
3939

40+
const platformMetadata = {
41+
name: 'platform',
42+
inputs: [
43+
{
44+
name: 'action',
45+
},
46+
{
47+
name: 'platform',
48+
}
49+
],
50+
options: [
51+
{
52+
name: 'boolopt',
53+
type: Boolean,
54+
default: false,
55+
},
56+
]
57+
};
58+
4059
describe('filterArgumentsForCordova', () => {
4160

4261
it('should return the command name and inputs if no options passed', () => {
4362
const options = { _: ['ios'], boolopt: false, cdvopt1: null, cdvopt2: false, prod: true, optimizejs: true };
44-
const result = filterArgumentsForCordova(metadata, options);
63+
const result = filterArgumentsForCordova(buildMetadata, options);
4564
expect(result).toEqual(['build', 'ios']);
4665
});
4766

4867
it('should only include options with the Cordova intent', () => {
4968
const options = { _: ['ios'], boolopt: true, cdvopt1: 'foo', cdvopt2: true, prod: true, optimizejs: true };
50-
const result = filterArgumentsForCordova(metadata, options);
69+
const result = filterArgumentsForCordova(buildMetadata, options);
5170
expect(result).toEqual(['build', 'ios', '--cdvopt1', 'foo', '--cdvopt2']);
5271
});
5372

5473
it('should include --verbose', () => {
5574
const options = { _: ['ios'], boolopt: true, cdvopt1: 'foo', cdvopt2: true, prod: true, optimizejs: true, verbose: true };
56-
const result = filterArgumentsForCordova(metadata, options);
75+
const result = filterArgumentsForCordova(buildMetadata, options);
5776
expect(result).toEqual(['build', 'ios', '--cdvopt1', 'foo', '--cdvopt2', '--verbose']);
5877
});
5978

6079
it('should include additional options', () => {
6180
const options = { _: ['android'], '--': ['--someopt'], boolopt: true, cdvopt1: 'foo', cdvopt2: true, prod: true, optimizejs: true };
62-
const result = filterArgumentsForCordova(metadata, options);
81+
const result = filterArgumentsForCordova(buildMetadata, options);
6382
expect(result).toEqual(['build', 'android', '--cdvopt1', 'foo', '--cdvopt2', '--someopt']);
6483
});
6584

6685
it('should include additional and unparsed options', () => {
6786
const options = { _: ['android'], '--': ['--someopt', '--', '--gradleArg=-PcdvBuildMultipleApks=true'], boolopt: true, cdvopt1: 'foo', cdvopt2: true, prod: true, optimizejs: true };
68-
const result = filterArgumentsForCordova(metadata, options);
87+
const result = filterArgumentsForCordova(buildMetadata, options);
6988
expect(result).toEqual(['build', 'android', '--cdvopt1', 'foo', '--cdvopt2', '--someopt', '--', '--gradleArg=-PcdvBuildMultipleApks=true']);
7089
});
7190

91+
it('should respect --nosave', () => {
92+
const options = { _: ['add', 'android'], '--': [], nosave: true };
93+
const result = filterArgumentsForCordova(platformMetadata, options);
94+
expect(result).toEqual(['platform', 'add', 'android', '--nosave']);
95+
});
96+
7297
});
7398

7499
describe('generateOptionsForCordovaBuild', () => {
@@ -77,15 +102,15 @@ describe('@ionic/cli-utils', () => {
77102
const inputs = ['ios'];
78103
const options = { _: [] };
79104

80-
const result = generateOptionsForCordovaBuild(metadata, inputs, options);
105+
const result = generateOptionsForCordovaBuild(buildMetadata, inputs, options);
81106
expect(result).toEqual({ '_': [], externalAddressRequired: true, nobrowser: true, engine: 'cordova', platform: 'ios' });
82107
});
83108

84109
it('should include the options with app-scripts group or no group, but not cordova group', () => {
85110
const inputs = ['ios'];
86111
const options = { _: [], boolopt: false, cdvopt1: null, cdvopt2: false, prod: true, optimizejs: true };
87112

88-
const result = generateOptionsForCordovaBuild(metadata, inputs, options);
113+
const result = generateOptionsForCordovaBuild(buildMetadata, inputs, options);
89114
expect(result).toEqual({ '_': [], boolopt: false, externalAddressRequired: true, nobrowser: true, engine: 'cordova', platform: 'ios', prod: true, optimizejs: true });
90115
});
91116

‎packages/@ionic/cli-utils/src/lib/integrations/cordova/project.ts

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,29 +1,9 @@
11
import * as path from 'path';
22

3-
import chalk from 'chalk';
4-
53
import { readDir } from '@ionic/cli-framework/utils/fs';
64

7-
import { IonicEnvironment } from '../../../definitions';
8-
import { FatalException } from '../../errors';
9-
105
export async function getPlatforms(projectDir: string): Promise<string[]> {
116
const platformsDir = path.resolve(projectDir, 'platforms');
127
const dirContents = await readDir(platformsDir);
138
return dirContents.filter(f => f && f !== 'platforms.json' && !f.startsWith('.'));
149
}
15-
16-
export async function installPlatform(env: IonicEnvironment, platform: string, cwd: string, extraArgs: string[] = []): Promise<void> {
17-
try {
18-
await env.shell.run('cordova', ['platform', 'add', platform, '--save', ...extraArgs], { fatalOnError: false, showError: false, cwd });
19-
} catch (e) {
20-
const s = String(e);
21-
22-
if (s.match(/Platform [A-Za-z0-9-]+ already added/)) {
23-
env.log.warn(`Platform already added. Saving platforms to ${chalk.bold('config.xml')}.`);
24-
await env.shell.run('cordova', ['platform', 'save'], { cwd });
25-
} else {
26-
throw new FatalException(s, typeof e.exitCode === 'undefined' ? 1 : e.exitCode);
27-
}
28-
}
29-
}

‎packages/@ionic/cli-utils/src/lib/integrations/cordova/utils.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,12 @@ export function filterArgumentsForCordova(metadata: CommandMetadata, options: Co
1919
type: Boolean,
2020
groups: ['cordova'],
2121
},
22+
{
23+
name: 'nosave',
24+
summary: '',
25+
type: Boolean,
26+
groups: ['cordova'],
27+
},
2228
];
2329

2430
m.options.push(...globalCordovaOpts);

‎packages/ionic/src/commands/cordova/base.ts

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -137,19 +137,29 @@ export abstract class CordovaCommand extends Command {
137137
}
138138
}
139139

140-
async checkForPlatformInstallation(runPlatform: string) {
140+
async checkForPlatformInstallation(platform: string, { promptToInstall = false, promptToInstallRefusalMsg = `Cannot run this command for the ${chalk.green(platform)} platform unless it is installed.` }: { promptToInstall?: boolean; promptToInstallRefusalMsg?: string; } = {}): Promise<void> {
141141
if (!this.project) {
142142
throw new FatalException('Cannot use Cordova outside a project directory.');
143143
}
144144

145-
if (runPlatform) {
146-
const { getPlatforms, installPlatform } = await import('@ionic/cli-utils/lib/integrations/cordova/project');
145+
if (platform) {
146+
const { getPlatforms } = await import('@ionic/cli-utils/lib/integrations/cordova/project');
147147

148148
const cordova = await this.project.getIntegration('cordova');
149149
const platforms = await getPlatforms(cordova.root);
150150

151-
if (!platforms.includes(runPlatform)) {
152-
await installPlatform(this.env, runPlatform, cordova.root);
151+
if (!platforms.includes(platform)) {
152+
const confirm = promptToInstall ? await this.env.prompt({
153+
message: `Platform ${chalk.green(platform)} is not installed! Would you like to install it?`,
154+
type: 'confirm',
155+
name: 'confirm',
156+
}) : true;
157+
158+
if (confirm) {
159+
await this.runCordova(['platform', 'add', platform, '--save']);
160+
} else {
161+
throw new FatalException(promptToInstallRefusalMsg);
162+
}
153163
}
154164
}
155165
}

‎packages/ionic/src/commands/cordova/platform.ts

Lines changed: 8 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,8 @@
11
import chalk from 'chalk';
2-
32
import * as lodash from 'lodash';
43

5-
import { contains, separateArgv, validate, validators } from '@ionic/cli-framework';
6-
import {
7-
CommandInstanceInfo,
8-
CommandLineInputs,
9-
CommandLineOptions,
10-
CommandMetadata,
11-
CommandPreRun,
12-
} from '@ionic/cli-utils';
4+
import { contains, validate, validators } from '@ionic/cli-framework';
5+
import { CommandInstanceInfo, CommandLineInputs, CommandLineOptions, CommandMetadata, CommandPreRun } from '@ionic/cli-utils';
136
import { FatalException } from '@ionic/cli-utils/lib/errors';
147
import { runCommand } from '@ionic/cli-utils/lib/executor';
158

@@ -100,17 +93,15 @@ Like running ${chalk.green('cordova platform')} directly, but adds default Ionic
10093

10194
const cordovaArgs = filterArgumentsForCordova(metadata, options);
10295

103-
if ((action === 'add' || action === 'remove') && lodash.intersection(cordovaArgs, ['--save', '--nosave', '--no-save']).length === 0) {
96+
if (
97+
(action === 'add' || action === 'remove') &&
98+
(options['save'] !== false && !options['nosave']) &&
99+
lodash.intersection(options['--'] || [], ['--save', '--nosave', '--no-save']).length === 0
100+
) {
104101
cordovaArgs.push('--save');
105102
}
106103

107-
if (action === 'add') {
108-
const { installPlatform } = await import('@ionic/cli-utils/lib/integrations/cordova/project');
109-
const [ , extraArgs ] = separateArgv(inputs);
110-
await installPlatform(this.env, platformName, cordova.root, extraArgs);
111-
} else {
112-
await this.runCordova(cordovaArgs, {});
113-
}
104+
await this.runCordova(cordovaArgs, {});
114105

115106
const isLoggedIn = this.env.session.isLoggedIn();
116107

‎packages/ionic/src/commands/cordova/plugin.ts

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import chalk from 'chalk';
2+
import * as lodash from 'lodash';
23

34
import { OptionGroup, contains, validate, validators } from '@ionic/cli-framework';
45
import { CommandInstanceInfo, CommandLineInputs, CommandLineOptions, CommandMetadata, CommandPreRun } from '@ionic/cli-utils';
@@ -72,13 +73,18 @@ Like running ${chalk.green('cordova plugin')} directly, but provides friendly ch
7273
}
7374

7475
async run(inputs: CommandLineInputs, options: CommandLineOptions): Promise<void> {
76+
const [ action ] = inputs;
7577
const metadata = await this.getMetadata();
76-
const optionList = filterArgumentsForCordova(metadata, options);
78+
const cordovaArgs = filterArgumentsForCordova(metadata, options);
7779

78-
if (!optionList.includes('--save')) {
79-
optionList.push('--save');
80+
if (
81+
(action === 'add' || action === 'remove') &&
82+
(options['save'] !== false && !options['nosave']) &&
83+
lodash.intersection(options['--'] || [], ['--save', '--nosave', '--no-save']).length === 0
84+
) {
85+
cordovaArgs.push('--save');
8086
}
8187

82-
await this.runCordova(optionList, {});
88+
await this.runCordova(cordovaArgs, {});
8389
}
8490
}

‎packages/ionic/src/commands/cordova/prepare.ts

Lines changed: 7 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -57,35 +57,19 @@ You may wish to use ${chalk.green('ionic cordova prepare')} if you run your proj
5757
}
5858

5959
async run(inputs: CommandLineInputs, options: CommandLineOptions): Promise<void> {
60-
const { getPlatforms, installPlatform } = await import('@ionic/cli-utils/lib/integrations/cordova/project');
61-
6260
const [ platform ] = inputs;
6361

6462
if (!this.project) {
6563
throw new FatalException(`Cannot run ${chalk.green('ionic cordova prepare')} outside a project directory.`);
6664
}
6765

68-
const cordova = await this.project.getIntegration('cordova');
69-
const platforms = await getPlatforms(cordova.root);
70-
71-
if (platform) {
72-
if (!platforms.includes(platform)) {
73-
const confirm = await this.env.prompt({
74-
message: `Platform ${chalk.green(platform)} is not installed! Would you like to install it?`,
75-
type: 'confirm',
76-
name: 'confirm',
77-
});
78-
79-
if (confirm) {
80-
await installPlatform(this.env, platform, cordova.root);
81-
} else {
82-
throw new FatalException(
83-
`Can't prepare for ${chalk.green(platform)} unless the platform is installed.\n` +
84-
`Did you mean just ${chalk.green('ionic cordova prepare')}?\n`
85-
);
86-
}
87-
}
88-
}
66+
await this.checkForPlatformInstallation(platform, {
67+
promptToInstall: true,
68+
promptToInstallRefusalMsg: (
69+
`Can't prepare for ${chalk.green(platform)} unless the platform is installed.\n` +
70+
`Did you mean just ${chalk.green('ionic cordova prepare')}?\n`
71+
),
72+
});
8973

9074
const metadata = await this.getMetadata();
9175

‎packages/ionic/src/commands/cordova/requirements.ts

Lines changed: 7 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -36,35 +36,19 @@ Like running ${chalk.green('cordova requirements')} directly, but provides frien
3636
}
3737

3838
async run(inputs: CommandLineInputs, options: CommandLineOptions): Promise<void> {
39-
const { getPlatforms, installPlatform } = await import('@ionic/cli-utils/lib/integrations/cordova/project');
40-
4139
const [ platform ] = inputs;
4240

4341
if (!this.project) {
4442
throw new FatalException(`Cannot run ${chalk.green('ionic cordova requirements')} outside a project directory.`);
4543
}
4644

47-
const cordova = await this.project.getIntegration('cordova');
48-
const platforms = await getPlatforms(cordova.root);
49-
50-
if (platform) {
51-
if (!platforms.includes(platform)) {
52-
const confirm = await this.env.prompt({
53-
message: `Platform ${chalk.green(platform)} is not installed! Would you like to install it?`,
54-
type: 'confirm',
55-
name: 'confirm',
56-
});
57-
58-
if (confirm) {
59-
await installPlatform(this.env, platform, cordova.root);
60-
} else {
61-
throw new FatalException(
62-
`Can't gather requirements for ${chalk.green(platform)} unless the platform is installed.\n` +
63-
`Did you mean just ${chalk.green('ionic cordova requirements')}?\n`
64-
);
65-
}
66-
}
67-
}
45+
await this.checkForPlatformInstallation(platform, {
46+
promptToInstall: true,
47+
promptToInstallRefusalMsg: (
48+
`Can't gather requirements for ${chalk.green(platform)} unless the platform is installed.\n` +
49+
`Did you mean just ${chalk.green('ionic cordova requirements')}?\n`
50+
),
51+
});
6852

6953
const metadata = await this.getMetadata();
7054

0 commit comments

Comments
 (0)