Skip to content

feat(@schematics/update): add packageGroup version map support #13124

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 15, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 40 additions & 14 deletions packages/schematics/update/update/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,8 @@ interface PackageInfo {
}

interface UpdateMetadata {
packageGroup: string[];
packageGroupName?: string;
packageGroup: { [ packageName: string ]: string };
requirements: { [packageName: string]: string };
migrations?: string;
}
Expand All @@ -88,9 +89,9 @@ function _updatePeerVersion(infoMap: Map<string, PackageInfo>, name: string, ran
return range;
}
if (maybePackageInfo.target) {
name = maybePackageInfo.target.updateMetadata.packageGroup[0] || name;
name = maybePackageInfo.target.updateMetadata.packageGroupName || name;
} else {
name = maybePackageInfo.installed.updateMetadata.packageGroup[0] || name;
name = maybePackageInfo.installed.updateMetadata.packageGroupName || name;
}

const maybeTransform = peerCompatibleWhitelist[name];
Expand Down Expand Up @@ -353,7 +354,7 @@ function _getUpdateMetadata(
const metadata = packageJson['ng-update'];

const result: UpdateMetadata = {
packageGroup: [],
packageGroup: {},
requirements: {},
};

Expand All @@ -363,15 +364,28 @@ function _getUpdateMetadata(

if (metadata['packageGroup']) {
const packageGroup = metadata['packageGroup'];
// Verify that packageGroup is an array of strings. This is not an error but we still warn
// the user and ignore the packageGroup keys.
if (!Array.isArray(packageGroup) || packageGroup.some(x => typeof x != 'string')) {
// Verify that packageGroup is an array of strings or an map of versions. This is not an error
// but we still warn the user and ignore the packageGroup keys.
if (Array.isArray(packageGroup) && packageGroup.every(x => typeof x == 'string')) {
result.packageGroup = packageGroup.reduce((group, name) => {
group[name] = packageJson.version;

return group;
}, result.packageGroup);
} else if (typeof packageGroup == 'object' && packageGroup
&& Object.values(packageGroup).every(x => typeof x == 'string')) {
result.packageGroup = packageGroup;
} else {
logger.warn(
`packageGroup metadata of package ${packageJson.name} is malformed. Ignoring.`,
);
} else {
result.packageGroup = packageGroup;
}

result.packageGroupName = Object.keys(result.packageGroup)[0];
}

if (typeof metadata['packageGroupName'] == 'string') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conditional set can be removed with the above changes.

result.packageGroupName = metadata['packageGroupName'];
}

if (metadata['requirements']) {
Expand Down Expand Up @@ -654,22 +668,34 @@ function _addPackageGroup(
return;
}

const packageGroup = ngUpdateMetadata['packageGroup'];
let packageGroup = ngUpdateMetadata['packageGroup'];
if (!packageGroup) {
return;
}
if (!Array.isArray(packageGroup) || packageGroup.some(x => typeof x != 'string')) {
if (Array.isArray(packageGroup) && !packageGroup.some(x => typeof x != 'string')) {
packageGroup = packageGroup.reduce((acc, curr) => {
acc[curr] = maybePackage;

return acc;
}, {} as { [name: string]: string });
}

// Only need to check if it's an object because we set it right the time before.
if (typeof packageGroup != 'object'
|| packageGroup === null
|| Object.values(packageGroup).some(v => typeof v != 'string')
) {
logger.warn(`packageGroup metadata of package ${npmPackageJson.name} is malformed.`);

return;
}

packageGroup
Object.keys(packageGroup)
.filter(name => !packages.has(name)) // Don't override names from the command line.
.filter(name => allDependencies.has(name)) // Remove packages that aren't installed.
.forEach(name => {
packages.set(name, maybePackage);
});
packages.set(name, packageGroup[name]);
});
}

/**
Expand Down
34 changes: 34 additions & 0 deletions packages/schematics/update/update/index_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,40 @@ describe('@schematics/update', () => {
).toPromise().then(done, done.fail);
}, 45000);

it('uses packageGroup for versioning', async () => {
// Add the basic migration package.
const content = virtualFs.fileBufferToString(host.sync.read(normalize('/package.json')));
const packageJson = JSON.parse(content);
const dependencies = packageJson['dependencies'];
dependencies['@angular-devkit-tests/update-package-group-1'] = '1.0.0';
dependencies['@angular-devkit-tests/update-package-group-2'] = '1.0.0';
host.sync.write(
normalize('/package.json'),
virtualFs.stringToFileBuffer(JSON.stringify(packageJson)),
);

await schematicRunner.runSchematicAsync('update', {
packages: ['@angular-devkit-tests/update-package-group-1'],
}, appTree).pipe(
map(tree => {
const packageJson = JSON.parse(tree.readContent('/package.json'));
const deps = packageJson['dependencies'];
expect(deps['@angular-devkit-tests/update-package-group-1']).toBe('1.2.0');
expect(deps['@angular-devkit-tests/update-package-group-2']).toBe('2.0.0');

// Check install task.
expect(schematicRunner.tasks).toEqual([
{
name: 'node-package',
options: jasmine.objectContaining({
command: 'install',
}),
},
]);
}),
).toPromise();
}, 45000);

it('can migrate only', done => {
// Add the basic migration package.
const content = virtualFs.fileBufferToString(host.sync.read(normalize('/package.json')));
Expand Down
14 changes: 14 additions & 0 deletions tests/schematics/update/packages/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@

## Update Package Group packages

* `[email protected]` -> `update-package-group2@^1`
* `[email protected]` -> `[email protected]`
----

* `[email protected]` -> `update-package-group2@^1`
* `[email protected]` -> `[email protected]`
----

* `[email protected]` -> `update-package-group2@^2`
* `[email protected]` -> `update-package-group1@^1`
----
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"name": "@angular-devkit-tests/update-package-group-1",
"version": "1.2.0",
"description": "Tests",
"ng-update": {
"packageGroup": {
"@angular-devkit-tests/update-package-group-1": "",
"@angular-devkit-tests/update-package-group-2": "^2"
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"name": "@angular-devkit-tests/update-package-group-2",
"version": "2.0.0",
"description": "Tests",
"ng-update": {
"packageGroup": {
"@angular-devkit-tests/update-package-group-1": "^1",
"@angular-devkit-tests/update-package-group-2": ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifying the owning package again in the group and with an empty version specifier is ambiguous. From an npm perspective this would mean install "any version" but the user doesn't want "any version". They want the version of the actual package specified above. They could put the version there as well but then there are two elements that need to be kept in sync within the same file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Angular does it and it's kind of suggested (since it's just easier to list all packages in all package.json). It's supported and it's ignored since it's on the command line.

}
}
}