[go: nahoru, domu]
Skip to content

Commit

Permalink
fix(@schematics/angular): only issue a warning for addDependency exis…
Browse files Browse the repository at this point in the history 
…ting specifier

When using the `addDependency` helper rule, a package specifier mismatch will
now result in a warning instead of an error. This prevents the potential
for breaking changes in schematics that previously allowed the behavior.
The warning will also display the existing and new specifiers to better inform
the schematic user of the outcome of the schematic.
  • Loading branch information
@clydin @dgp1130
clydin authored and dgp1130 committed Jul 13, 2022
1 parent 5708ac3 commit b8bf3b4
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 24 deletions.
22 changes: 16 additions & 6 deletions packages/schematics/angular/utility/dependency.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,13 +103,23 @@ export function addDependency(
if (!dependencySection) {
// Section is not present. The dependency can be added to a new object literal for the section.
manifest[type] = { [name]: specifier };
} else if (dependencySection[name] === specifier) {
// Already present with same specifier
return;
} else if (dependencySection[name]) {
// Already present but different specifier
throw new Error(`Package dependency "${name}" already exists with a different specifier.`);
} else {
const existingSpecifier = dependencySection[name];

if (existingSpecifier === specifier) {
// Already present with same specifier
return;
}

if (existingSpecifier) {
// Already present but different specifier
// This warning may become an error in the future
context.logger.warn(
`Package dependency "${name}" already exists with a different specifier. ` +
`"${existingSpecifier}" will be replaced with "${specifier}".`,
);
}

// Add new dependency in alphabetical order
const entries = Object.entries(dependencySection);
entries.push([name, specifier]);
Expand Down
55 changes: 37 additions & 18 deletions packages/schematics/angular/utility/dependency_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,31 @@ import {
} from '@angular-devkit/schematics';
import { DependencyType, InstallBehavior, addDependency } from './dependency';

async function testRule(rule: Rule, tree: Tree): Promise<TaskConfigurationGenerator[]> {
interface LogEntry {
type: 'warn';
message: string;
}

async function testRule(
rule: Rule,
tree: Tree,
): Promise<{ tasks: TaskConfigurationGenerator[]; logs: LogEntry[] }> {
const tasks: TaskConfigurationGenerator[] = [];
const logs: LogEntry[] = [];
const context = {
addTask(task: TaskConfigurationGenerator) {
tasks.push(task);
},
logger: {
warn(message: string): void {
logs.push({ type: 'warn', message });
},
},
};

await callRule(rule, tree, context as unknown as SchematicContext).toPromise();

return tasks;
return { tasks, logs };
}

describe('addDependency', () => {
Expand All @@ -49,7 +63,7 @@ describe('addDependency', () => {
});
});

it('throws if a package is already present with a different specifier', async () => {
it('warns if a package is already present with a different specifier', async () => {
const tree = new EmptyTree();
tree.create(
'/package.json',
Expand All @@ -60,9 +74,14 @@ describe('addDependency', () => {

const rule = addDependency('@angular/core', '^14.0.0');

await expectAsync(testRule(rule, tree)).toBeRejectedWithError(
undefined,
'Package dependency "@angular/core" already exists with a different specifier.',
const { logs } = await testRule(rule, tree);
expect(logs).toContain(
jasmine.objectContaining({
type: 'warn',
message:
'Package dependency "@angular/core" already exists with a different specifier. ' +
'"^13.0.0" will be replaced with "^14.0.0".',
}),
);
});

Expand Down Expand Up @@ -164,7 +183,7 @@ describe('addDependency', () => {

const rule = addDependency('@angular/core', '^14.0.0');

const tasks = await testRule(rule, tree);
const { tasks } = await testRule(rule, tree);

expect(tasks.map((task) => task.toConfiguration())).toEqual([
{
Expand All @@ -182,7 +201,7 @@ describe('addDependency', () => {
packageJsonPath: '/abc/package.json',
});

const tasks = await testRule(rule, tree);
const { tasks } = await testRule(rule, tree);

expect(tasks.map((task) => task.toConfiguration())).toEqual([
{
Expand All @@ -201,7 +220,7 @@ describe('addDependency', () => {
install: InstallBehavior.Auto,
});

const tasks = await testRule(rule, tree);
const { tasks } = await testRule(rule, tree);

expect(tasks.map((task) => task.toConfiguration())).toEqual([
{
Expand All @@ -220,7 +239,7 @@ describe('addDependency', () => {
install: InstallBehavior.Always,
});

const tasks = await testRule(rule, tree);
const { tasks } = await testRule(rule, tree);

expect(tasks.map((task) => task.toConfiguration())).toEqual([
{
Expand All @@ -239,7 +258,7 @@ describe('addDependency', () => {
install: InstallBehavior.None,
});

const tasks = await testRule(rule, tree);
const { tasks } = await testRule(rule, tree);

expect(tasks).toEqual([]);
});
Expand All @@ -255,7 +274,7 @@ describe('addDependency', () => {

const rule = addDependency('@angular/core', '^14.0.0');

const tasks = await testRule(rule, tree);
const { tasks } = await testRule(rule, tree);

expect(tasks).toEqual([]);
});
Expand All @@ -269,7 +288,7 @@ describe('addDependency', () => {
addDependency('@angular/common', '^14.0.0'),
]);

const tasks = await testRule(rule, tree);
const { tasks } = await testRule(rule, tree);

expect(tasks.map((task) => task.toConfiguration())).toEqual([
{
Expand All @@ -288,7 +307,7 @@ describe('addDependency', () => {
addDependency('@angular/common', '^14.0.0', { install: InstallBehavior.Auto }),
]);

const tasks = await testRule(rule, tree);
const { tasks } = await testRule(rule, tree);

expect(tasks.map((task) => task.toConfiguration())).toEqual([
{
Expand All @@ -307,7 +326,7 @@ describe('addDependency', () => {
addDependency('@angular/common', '^14.0.0', { install: InstallBehavior.None }),
]);

const tasks = await testRule(rule, tree);
const { tasks } = await testRule(rule, tree);

expect(tasks.map((task) => task.toConfiguration())).toEqual([
{
Expand All @@ -326,7 +345,7 @@ describe('addDependency', () => {
addDependency('@angular/common', '^14.0.0', { install: InstallBehavior.Auto }),
]);

const tasks = await testRule(rule, tree);
const { tasks } = await testRule(rule, tree);

expect(tasks.map((task) => task.toConfiguration())).toEqual([
{
Expand All @@ -345,7 +364,7 @@ describe('addDependency', () => {
addDependency('@angular/common', '^14.0.0', { install: InstallBehavior.Always }),
]);

const tasks = await testRule(rule, tree);
const { tasks } = await testRule(rule, tree);

expect(tasks.map((task) => task.toConfiguration())).toEqual([
{
Expand All @@ -369,7 +388,7 @@ describe('addDependency', () => {
addDependency('@angular/common', '^14.0.0', { packageJsonPath: '/abc/package.json' }),
]);

const tasks = await testRule(rule, tree);
const { tasks } = await testRule(rule, tree);

expect(tasks.map((task) => task.toConfiguration())).toEqual([
{
Expand Down

0 comments on commit b8bf3b4

Please sign in to comment.