diff --git a/packages/compiler-cli/ngcc/src/execution/cluster/package_json_updater.ts b/packages/compiler-cli/ngcc/src/execution/cluster/package_json_updater.ts index aae19dd275..6014045507 100644 --- a/packages/compiler-cli/ngcc/src/execution/cluster/package_json_updater.ts +++ b/packages/compiler-cli/ngcc/src/execution/cluster/package_json_updater.ts @@ -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'); } } diff --git a/packages/compiler-cli/ngcc/src/packages/build_marker.ts b/packages/compiler-cli/ngcc/src/packages/build_marker.ts index 26d96d44f7..61caa52e32 100644 --- a/packages/compiler-cli/ngcc/src/packages/build_marker.ts +++ b/packages/compiler-cli/ngcc/src/packages/build_marker.ts @@ -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 diff --git a/packages/compiler-cli/ngcc/src/writing/new_entry_point_file_writer.ts b/packages/compiler-cli/ngcc/src/writing/new_entry_point_file_writer.ts index f21d64b049..3a48256a2a 100644 --- a/packages/compiler-cli/ngcc/src/writing/new_entry_point_file_writer.ts +++ b/packages/compiler-cli/ngcc/src/writing/new_entry_point_file_writer.ts @@ -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); diff --git a/packages/compiler-cli/ngcc/src/writing/package_json_updater.ts b/packages/compiler-cli/ngcc/src/writing/package_json_updater.ts index 9295191efd..bfd5295ad4 100644 --- a/packages/compiler-cli/ngcc/src/writing/package_json_updater.ts +++ b/packages/compiler-cli/ngcc/src/writing/package_json_updater.ts @@ -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; + } } diff --git a/packages/compiler-cli/ngcc/test/execution/cluster/package_json_updater_spec.ts b/packages/compiler-cli/ngcc/test/execution/cluster/package_json_updater_spec.ts index e5bd88cd5f..d2cea43299 100644 --- a/packages/compiler-cli/ngcc/test/execution/cluster/package_json_updater_spec.ts +++ b/packages/compiler-cli/ngcc/test/execution/cluster/package_json_updater_spec.ts @@ -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']], }); }); diff --git a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts index dcfb6e3e25..4169641809 100644 --- a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts +++ b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts @@ -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', () => { diff --git a/packages/compiler-cli/ngcc/test/writing/package_json_updater_spec.ts b/packages/compiler-cli/ngcc/test/writing/package_json_updater_spec.ts index 1a3c5dd346..56f6a9a643 100644 --- a/packages/compiler-cli/ngcc/test/writing/package_json_updater_spec.ts +++ b/packages/compiler-cli/ngcc/test/writing/package_json_updater_spec.ts @@ -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', + })); + }); + }); }); });