iSharkFly-Docs
/
angular-docs-cn
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(ngcc): update `package.json` deterministically (#34870) 

Ngcc adds properties to the `package.json` files of the entry-points it
processes to mark them as processed for a format and point to the
created Ivy entry-points (in case of `--create-ivy-entry-points`). When
running ngcc in parallel mode (which is the default for the standalone
ngcc command), multiple formats can be processed simultaneously for the
same entry-point and the order of completion is not deterministic.

Previously, ngcc would append new properties at the end of the target
object in `package.json` as soon as the format processing was completed.
As a result, the order of properties in the resulting `package.json`
(when processing multiple formats for an entry-point in parallel) was
not deterministic. For tools that use file hashes for caching purposes
(such as Bazel), this lead to a high probability of cache misses.

This commit fixes the problem by ensuring that the position of
properties added to `package.json` files is deterministic and
independent of the order in which each format is processed.

Jira issue: [FW-1801](https://angular-team.atlassian.net/browse/FW-1801)

Fixes #34635

PR Close #34870
This commit is contained in:
George Kalpakas 2020-01-20 18:02:12 +02:00 committed by Andrew Kushnir
parent 43db4ffcd6
commit 93ffc67bfb
7 changed files with 314 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
packages/compiler-cli/ngcc/src/execution/cluster/package_json_updater.ts
View File

@ -44,7 +44,8 @@ export class ClusterPackageJsonUpdater implements PackageJsonUpdater {
throw new Error(`Missing property path for writing value to '${packageJsonPath}'.`);
}
applyChange(preExistingParsedJson, propPath, value);
// No need to take property positioning into account for in-memory representations.
applyChange(preExistingParsedJson, propPath, value, 'unimportant');
}
}

2
packages/compiler-cli/ngcc/src/packages/build_marker.ts
View File

@ -60,7 +60,7 @@ export function markAsProcessed(
// Update the format properties to mark them as processed.
for (const prop of formatProperties) {
update.addChange(['__processed_by_ivy_ngcc__', prop], NGCC_VERSION);
update.addChange(['__processed_by_ivy_ngcc__', prop], NGCC_VERSION, 'alphabetic');
}
// Update the `prepublishOnly` script (keeping a backup, if necessary) to prevent `ngcc`'d

2
packages/compiler-cli/ngcc/src/writing/new_entry_point_file_writer.ts
View File

@ -93,7 +93,7 @@ export class NewEntryPointFileWriter extends InPlaceFileWriter {
`(${formatProperties.join(', ')}) map to more than one format-path.`);
}
update.addChange([`${formatProperty}_ivy_ngcc`], newFormatPath);
update.addChange([`${formatProperty}_ivy_ngcc`], newFormatPath, {before: formatProperty});
}
update.writeChanges(packageJsonPath, packageJson);

83
packages/compiler-cli/ngcc/src/writing/package_json_updater.ts
View File

@ -10,7 +10,8 @@ import {AbsoluteFsPath, FileSystem, dirname} from '../../../src/ngtsc/file_syste
import {JsonObject, JsonValue} from '../packages/entry_point';
export type PackageJsonChange = [string[], JsonValue];
export type PackageJsonChange = [string[], JsonValue, PackageJsonPropertyPositioning];
export type PackageJsonPropertyPositioning = 'unimportant' | 'alphabetic' | {before: string};
export type WritePackageJsonChangesFn =
(changes: PackageJsonChange[], packageJsonPath: AbsoluteFsPath, parsedJson?: JsonObject) =>
void;
@ -23,8 +24,9 @@ export type WritePackageJsonChangesFn =
* const updatePackageJson = packageJsonUpdater
* .createUpdate()
* .addChange(['name'], 'package-foo')
* .addChange(['scripts', 'foo'], 'echo FOOOO...')
* .addChange(['dependencies', 'bar'], '1.0.0')
* .addChange(['scripts', 'foo'], 'echo FOOOO...', 'unimportant')
* .addChange(['dependencies', 'baz'], '1.0.0', 'alphabetic')
* .addChange(['dependencies', 'bar'], '2.0.0', {before: 'baz'})
* .writeChanges('/foo/package.json');
* // or
* // .writeChanges('/foo/package.json', inMemoryParsedJson);
@ -33,13 +35,13 @@ export type WritePackageJsonChangesFn =
export interface PackageJsonUpdater {
/**
* Create a `PackageJsonUpdate` object, which provides a fluent API for batching updates to a
* `package.json` file. (Batching the updates is useful, because it avoid unnecessary I/O
* `package.json` file. (Batching the updates is useful, because it avoids unnecessary I/O
* operations.)
*/
createUpdate(): PackageJsonUpdate;
/**
* Write a set of changes to the specified `package.json` file and (and optionally a pre-existing,
* Write a set of changes to the specified `package.json` file (and optionally a pre-existing,
* in-memory representation of it).
*
* @param changes The set of changes to apply.
@ -65,15 +67,27 @@ export class PackageJsonUpdate {
constructor(private writeChangesImpl: WritePackageJsonChangesFn) {}
/**
* Record a change to a `package.json` property. If the ancestor objects do not yet exist in the
* `package.json` file, they will be created.
* Record a change to a `package.json` property.
*
* @param propertyPath The path of a (possibly nested) property to update.
* If the ancestor objects do not yet exist in the `package.json` file, they will be created. The
* positioning of the property can also be specified. (If the property already exists, it will be
* moved accordingly.)
*
* NOTE: Property positioning is only guaranteed to be respected in the serialized `package.json`
* file. Positioning will not be taken into account when updating in-memory representations.
*
* NOTE 2: Property positioning only affects the last property in `propertyPath`. Ancestor
* objects' positioning will not be affected.
*
* @param propertyPath The path of a (possibly nested) property to add/update.
* @param value The new value to set the property to.
* @param position The desired position for the added/updated property.
*/
addChange(propertyPath: string[], value: JsonValue): this {
addChange(
propertyPath: string[], value: JsonValue,
positioning: PackageJsonPropertyPositioning = 'unimportant'): this {
this.ensureNotApplied();
this.changes.push([propertyPath, value]);
this.changes.push([propertyPath, value, positioning]);
return this;
}
@ -121,15 +135,16 @@ export class DirectPackageJsonUpdater implements PackageJsonUpdater {
// Apply all changes to both the canonical representation (read from disk) and any pre-existing,
// in-memory representation.
for (const [propPath, value] of changes) {
for (const [propPath, value, positioning] of changes) {
if (propPath.length === 0) {
throw new Error(`Missing property path for writing value to '${packageJsonPath}'.`);
}
applyChange(parsedJson, propPath, value);
applyChange(parsedJson, propPath, value, positioning);
if (preExistingParsedJson) {
applyChange(preExistingParsedJson, propPath, value);
// No need to take property positioning into account for in-memory representations.
applyChange(preExistingParsedJson, propPath, value, 'unimportant');
}
}
@ -141,7 +156,9 @@ export class DirectPackageJsonUpdater implements PackageJsonUpdater {
}
// Helpers
export function applyChange(ctx: JsonObject, propPath: string[], value: JsonValue): void {
export function applyChange(
ctx: JsonObject, propPath: string[], value: JsonValue,
positioning: PackageJsonPropertyPositioning): void {
const lastPropIdx = propPath.length - 1;
const lastProp = propPath[lastPropIdx];
@ -157,4 +174,42 @@ export function applyChange(ctx: JsonObject, propPath: string[], value: JsonValu
}
ctx[lastProp] = value;
positionProperty(ctx, lastProp, positioning);
}
function movePropBefore(ctx: JsonObject, prop: string, isNextProp: (p: string) => boolean): void {
const allProps = Object.keys(ctx);
const otherProps = allProps.filter(p => p !== prop);
const nextPropIdx = otherProps.findIndex(isNextProp);
const propsToShift = (nextPropIdx === -1) ? [] : otherProps.slice(nextPropIdx);
movePropToEnd(ctx, prop);
propsToShift.forEach(p => movePropToEnd(ctx, p));
}
function movePropToEnd(ctx: JsonObject, prop: string): void {
const value = ctx[prop];
delete ctx[prop];
ctx[prop] = value;
}
function positionProperty(
ctx: JsonObject, prop: string, positioning: PackageJsonPropertyPositioning): void {
switch (positioning) {
case 'alphabetic':
movePropBefore(ctx, prop, p => p > prop);
break;
case 'unimportant':
// Leave the property order unchanged; i.e. newly added properties will be last and existing
// ones will remain in their old position.
break;
default:
if ((typeof positioning !== 'object') || (positioning.before === undefined)) {
throw new Error(
`Unknown positioning (${JSON.stringify(positioning)}) for property '${prop}'.`);
}
movePropBefore(ctx, prop, p => p === positioning.before);
break;
}
}

44
packages/compiler-cli/ngcc/test/execution/cluster/package_json_updater_spec.ts
View File

@ -14,7 +14,7 @@ import {absoluteFrom as _} from '../../../../src/ngtsc/file_system';
import {runInEachFileSystem} from '../../../../src/ngtsc/file_system/testing';
import {ClusterPackageJsonUpdater} from '../../../src/execution/cluster/package_json_updater';
import {JsonObject} from '../../../src/packages/entry_point';
import {PackageJsonUpdate, PackageJsonUpdater} from '../../../src/writing/package_json_updater';
import {PackageJsonPropertyPositioning, PackageJsonUpdate, PackageJsonUpdater} from '../../../src/writing/package_json_updater';
import {mockProperty} from '../../helpers/spy_utils';
@ -48,10 +48,18 @@ runInEachFileSystem(() => {
const update = updater.createUpdate();
update.addChange(['foo'], 'updated');
update.addChange(['baz'], 'updated 2', 'alphabetic');
update.addChange(['bar'], 'updated 3', {before: 'bar'});
update.writeChanges(jsonPath);
expect(writeChangesSpy)
.toHaveBeenCalledWith([[['foo'], 'updated']], jsonPath, undefined);
.toHaveBeenCalledWith(
[
[['foo'], 'updated', 'unimportant'],
[['baz'], 'updated 2', 'alphabetic'],
[['bar'], 'updated 3', {before: 'bar'}],
],
jsonPath, undefined);
});
}));
});
@ -65,10 +73,18 @@ runInEachFileSystem(() => {
const jsonPath = _('/foo/package.json');
const parsedJson = {foo: 'bar'};
updater.createUpdate().addChange(['foo'], 'updated').writeChanges(jsonPath, parsedJson);
updater.createUpdate()
.addChange(['foo'], 'updated')
.addChange(['bar'], 'updated too', 'alphabetic')
.writeChanges(jsonPath, parsedJson);
expect(delegate.writeChanges)
.toHaveBeenCalledWith([[['foo'], 'updated']], jsonPath, parsedJson);
.toHaveBeenCalledWith(
[
[['foo'], 'updated', 'unimportant'],
[['bar'], 'updated too', 'alphabetic'],
],
jsonPath, parsedJson);
});
it('should throw, if trying to re-apply an already applied update', () => {
@ -89,21 +105,31 @@ runInEachFileSystem(() => {
it('should send an `update-package-json` message to the master process', () => {
const jsonPath = _('/foo/package.json');
const writeToProp = (propPath: string[], parsed?: JsonObject) =>
updater.createUpdate().addChange(propPath, 'updated').writeChanges(jsonPath, parsed);
const writeToProp =
(propPath: string[], positioning?: PackageJsonPropertyPositioning,
parsed?: JsonObject) => updater.createUpdate()
.addChange(propPath, 'updated', positioning)
.writeChanges(jsonPath, parsed);
writeToProp(['foo']);
expect(processSendSpy).toHaveBeenCalledWith({
type: 'update-package-json',
packageJsonPath: jsonPath,
changes: [[['foo'], 'updated']],
changes: [[['foo'], 'updated', 'unimportant']],
});
writeToProp(['bar', 'baz', 'qux'], {});
writeToProp(['bar'], {before: 'foo'});
expect(processSendSpy).toHaveBeenCalledWith({
type: 'update-package-json',
packageJsonPath: jsonPath,
changes: [[['bar', 'baz', 'qux'], 'updated']],
changes: [[['bar'], 'updated', {before: 'foo'}]],
});
writeToProp(['bar', 'baz', 'qux'], 'alphabetic', {});
expect(processSendSpy).toHaveBeenCalledWith({
type: 'update-package-json',
packageJsonPath: jsonPath,
changes: [[['bar', 'baz', 'qux'], 'updated', 'alphabetic']],
});
});

73
packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts
View File

@ -760,6 +760,79 @@ runInEachFileSystem(() => {
expect(pkg.fesm5_ivy_ngcc).toEqual('__ivy_ngcc__/fesm5/core.js');
expect(pkg.module_ivy_ngcc).toEqual('__ivy_ngcc__/fesm5/core.js');
});
it('should update `package.json` deterministically (regardless of entry-point processing order)',
() => {
// Ensure formats are not marked as processed in `package.json` at the beginning.
let pkg = loadPackage('@angular/core');
expectNotToHaveProp(pkg, 'esm5_ivy_ngcc');
expectNotToHaveProp(pkg, 'fesm2015_ivy_ngcc');
expectNotToHaveProp(pkg, 'fesm5_ivy_ngcc');
expectNotToHaveProp(pkg, '__processed_by_ivy_ngcc__');
// Process `fesm2015` and update `package.json`.
pkg = processFormatAndUpdatePackageJson('fesm2015');
expectNotToHaveProp(pkg, 'esm5_ivy_ngcc');
expectToHaveProp(pkg, 'fesm2015_ivy_ngcc');
expectNotToHaveProp(pkg, 'fesm5_ivy_ngcc');
expectToHaveProp(pkg.__processed_by_ivy_ngcc__ !, 'fesm2015');
// Process `fesm5` and update `package.json`.
pkg = processFormatAndUpdatePackageJson('fesm5');
expectNotToHaveProp(pkg, 'esm5_ivy_ngcc');
expectToHaveProp(pkg, 'fesm2015_ivy_ngcc');
expectToHaveProp(pkg, 'fesm5_ivy_ngcc');
expectToHaveProp(pkg.__processed_by_ivy_ngcc__ !, 'fesm5');
// Process `esm5` and update `package.json`.
pkg = processFormatAndUpdatePackageJson('esm5');
expectToHaveProp(pkg, 'esm5_ivy_ngcc');
expectToHaveProp(pkg, 'fesm2015_ivy_ngcc');
expectToHaveProp(pkg, 'fesm5_ivy_ngcc');
expectToHaveProp(pkg.__processed_by_ivy_ngcc__ !, 'esm5');
// Ensure the properties are in deterministic order (regardless of processing order).
const pkgKeys = stringifyKeys(pkg);
expect(pkgKeys).toContain('|esm5_ivy_ngcc|esm5|');
expect(pkgKeys).toContain('|fesm2015_ivy_ngcc|fesm2015|');
expect(pkgKeys).toContain('|fesm5_ivy_ngcc|fesm5|');
// NOTE:
// Along with the first format that is processed, the typings are processed as well.
// Also, once a property has been processed, alias properties as also marked as
// processed. Aliases properties are properties that point to the same entry-point file.
// For example:
// - `fesm2015` <=> `es2015`
// - `fesm5` <=> `module`
expect(stringifyKeys(pkg.__processed_by_ivy_ngcc__ !))
.toBe('|es2015|esm5|fesm2015|fesm5|module|typings|');
// Helpers
function expectNotToHaveProp(obj: object, prop: string) {
expect(obj.hasOwnProperty(prop))
.toBe(
false,
`Expected object not to have property '${prop}': ${JSON.stringify(obj, null, 2)}`);
}
function expectToHaveProp(obj: object, prop: string) {
expect(obj.hasOwnProperty(prop))
.toBe(
true,
`Expected object to have property '${prop}': ${JSON.stringify(obj, null, 2)}`);
}
function processFormatAndUpdatePackageJson(formatProp: string) {
mainNgcc({
basePath: '/node_modules/@angular/core',
createNewEntryPointFormats: true,
propertiesToConsider: [formatProp],
});
return loadPackage('@angular/core');
}
function stringifyKeys(obj: object) { return `|${Object.keys(obj).join('|')}|`; }
});
});
describe('diagnostics', () => {

133
packages/compiler-cli/ngcc/test/writing/package_json_updater_spec.ts
View File

@ -8,6 +8,7 @@
import {AbsoluteFsPath, FileSystem, absoluteFrom, getFileSystem} from '../../../src/ngtsc/file_system';
import {runInEachFileSystem} from '../../../src/ngtsc/file_system/testing';
import {loadTestFiles} from '../../../test/helpers';
import {JsonObject} from '../../src/packages/entry_point';
import {DirectPackageJsonUpdater, PackageJsonUpdater} from '../../src/writing/package_json_updater';
runInEachFileSystem(() => {
@ -151,5 +152,137 @@ runInEachFileSystem(() => {
expect(() => update.writeChanges(_('/bar/package.json')))
.toThrowError('Trying to apply a `PackageJsonUpdate` that has already been applied.');
});
describe('(property positioning)', () => {
// Helpers
const createJsonFile = (jsonObj: JsonObject) => {
const jsonPath = _('/foo/package.json');
loadTestFiles([{name: jsonPath, contents: JSON.stringify(jsonObj)}]);
return jsonPath;
};
const expectJsonEquals = (jsonFilePath: AbsoluteFsPath, jsonObj: JsonObject) =>
expect(fs.readFile(jsonFilePath).trim()).toBe(JSON.stringify(jsonObj, null, 2));
it('should not change property positioning by default', () => {
const jsonPath = createJsonFile({
p2: '2',
p1: {p12: '1.2', p11: '1.1'},
});
updater.createUpdate()
.addChange(['p1', 'p11'], '1.1-updated')
.addChange(['p1', 'p10'], '1.0-added')
.addChange(['p2'], '2-updated')
.addChange(['p0'], '0-added')
.writeChanges(jsonPath);
expectJsonEquals(jsonPath, {
p2: '2-updated',
p1: {p12: '1.2', p11: '1.1-updated', p10: '1.0-added'},
p0: '0-added',
});
});
it('should not change property positioning with `positioning: unimportant`', () => {
const jsonPath = createJsonFile({
p2: '2',
p1: {p12: '1.2', p11: '1.1'},
});
updater.createUpdate()
.addChange(['p1', 'p11'], '1.1-updated', 'unimportant')
.addChange(['p1', 'p10'], '1.0-added', 'unimportant')
.addChange(['p2'], '2-updated', 'unimportant')
.addChange(['p0'], '0-added', 'unimportant')
.writeChanges(jsonPath);
expectJsonEquals(jsonPath, {
p2: '2-updated',
p1: {p12: '1.2', p11: '1.1-updated', p10: '1.0-added'},
p0: '0-added',
});
});
it('should position added/updated properties alphabetically with `positioning: alphabetic`',
() => {
const jsonPath = createJsonFile({
p2: '2',
p1: {p12: '1.2', p11: '1.1'},
});
updater.createUpdate()
.addChange(['p1', 'p11'], '1.1-updated', 'alphabetic')
.addChange(['p1', 'p10'], '1.0-added', 'alphabetic')
.addChange(['p0'], '0-added', 'alphabetic')
.addChange(['p3'], '3-added', 'alphabetic')
.writeChanges(jsonPath);
expectJsonEquals(jsonPath, {
p0: '0-added',
p2: '2',
p1: {p10: '1.0-added', p11: '1.1-updated', p12: '1.2'},
p3: '3-added',
});
});
it('should position added/updated properties correctly with `positioning: {before: ...}`',
() => {
const jsonPath = createJsonFile({
p2: '2',
p1: {p12: '1.2', p11: '1.1'},
});
updater.createUpdate()
.addChange(['p0'], '0-added', {before: 'p1'})
.addChange(['p1', 'p10'], '1.0-added', {before: 'p11'})
.addChange(['p1', 'p12'], '1.2-updated', {before: 'p11'})
.writeChanges(jsonPath);
expectJsonEquals(jsonPath, {
p2: '2',
p0: '0-added',
p1: {p10: '1.0-added', p12: '1.2-updated', p11: '1.1'},
});
// Verify that trying to add before non-existent property, puts updated property at the
// end.
updater.createUpdate()
.addChange(['p3'], '3-added', {before: 'non-existent'})
.addChange(['p1', 'p10'], '1.0-updated', {before: 'non-existent'})
.writeChanges(jsonPath);
expectJsonEquals(jsonPath, {
p2: '2',
p0: '0-added',
p1: {p12: '1.2-updated', p11: '1.1', p10: '1.0-updated'},
p3: '3-added',
});
});
it('should ignore positioning when updating an in-memory representation', () => {
const jsonObj = {
p20: '20',
p10: {p102: '10.2', p101: '10.1'},
};
const jsonPath = createJsonFile(jsonObj);
updater.createUpdate()
.addChange(['p0'], '0-added', 'alphabetic')
.addChange(['p1'], '1-added', 'unimportant')
.addChange(['p2'], '2-added')
.addChange(['p20'], '20-updated', 'alphabetic')
.addChange(['p10', 'p103'], '10.3-added', {before: 'p102'})
.addChange(['p10', 'p102'], '10.2-updated', {before: 'p103'})
.writeChanges(jsonPath, jsonObj);
expect(JSON.stringify(jsonObj)).toBe(JSON.stringify({
p20: '20-updated',
p10: {p102: '10.2-updated', p101: '10.1', p103: '10.3-added'},
p0: '0-added',
p1: '1-added',
p2: '2-added',
}));
});
});
});
});