From 68d37ef0c1b8cfffcd5f7930f86ba4d4754324ce Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Mon, 4 Jun 2018 11:43:15 +0100 Subject: [PATCH] build(aio): ensure the correct decorator properties are merged (#24289) Previously only the `description` and `usageNotes` were being copied over from the call-member of the decorator interface. Important properties such as `shortDescription` were missed. These are now added and the code has been refactored to make it simpler and clearer to update which properties get copied as the requirements change. PR Close #24289 --- .../transforms/angular-api-package/index.js | 11 +++++++ .../processors/mergeDecoratorDocs.js | 8 +++-- .../processors/mergeDecoratorDocs.spec.js | 25 +++++++++------ aio/tools/transforms/helpers/utils.js | 16 +++++++++- aio/tools/transforms/helpers/utils.spec.js | 32 ++++++++++++++++++- 5 files changed, 79 insertions(+), 13 deletions(-) diff --git a/aio/tools/transforms/angular-api-package/index.js b/aio/tools/transforms/angular-api-package/index.js index 6a9d64cbaa..6eef84a819 100644 --- a/aio/tools/transforms/angular-api-package/index.js +++ b/aio/tools/transforms/angular-api-package/index.js @@ -119,6 +119,17 @@ module.exports = new Package('angular-api', [basePackage, typeScriptPackage]) addNotYetDocumentedProperty.docTypes = API_DOC_TYPES; }) + .config(function(mergeDecoratorDocs) { + mergeDecoratorDocs.propertiesToMerge = [ + 'shortDescription', + 'description', + 'security', + 'deprecated', + 'see', + 'usageNotes', + ]; + }) + .config(function(checkContentRules, EXPORT_DOC_TYPES) { // Min length rules const createMinLengthRule = require('./content-rules/minLength'); diff --git a/aio/tools/transforms/angular-api-package/processors/mergeDecoratorDocs.js b/aio/tools/transforms/angular-api-package/processors/mergeDecoratorDocs.js index 86571ea5fa..b15b28bc98 100644 --- a/aio/tools/transforms/angular-api-package/processors/mergeDecoratorDocs.js +++ b/aio/tools/transforms/angular-api-package/processors/mergeDecoratorDocs.js @@ -1,3 +1,5 @@ +const {mergeProperties} = require('../../helpers/utils'); + /** * Decorators in the Angular code base are made up from three code items: * @@ -47,6 +49,7 @@ module.exports = function mergeDecoratorDocs(log) { return { $runAfter: ['processing-docs'], $runBefore: ['docs-processed'], + propertiesToMerge: [], makeDecoratorCalls: [ {type: '', description: 'toplevel', functionName: 'makeDecorator'}, {type: 'Prop', description: 'property', functionName: 'makePropDecorator'}, @@ -55,6 +58,7 @@ module.exports = function mergeDecoratorDocs(log) { $process: function(docs) { var makeDecoratorCalls = this.makeDecoratorCalls; + var propertiesToMerge = this.propertiesToMerge; var docsToMerge = Object.create(null); docs.forEach(function(doc) { @@ -88,9 +92,9 @@ module.exports = function mergeDecoratorDocs(log) { log.debug( 'mergeDecoratorDocs: merging', doc.name, 'into', decoratorDoc.name, callMember.description.substring(0, 50)); + // Merge the documentation found in this call signature into the original decorator - decoratorDoc.description = callMember.description; - decoratorDoc.usageNotes = callMember.usageNotes; + mergeProperties(decoratorDoc, callMember, propertiesToMerge); // remove doc from its module doc's exports doc.moduleDoc.exports = diff --git a/aio/tools/transforms/angular-api-package/processors/mergeDecoratorDocs.spec.js b/aio/tools/transforms/angular-api-package/processors/mergeDecoratorDocs.spec.js index 77b3f2593d..77bc9f8141 100644 --- a/aio/tools/transforms/angular-api-package/processors/mergeDecoratorDocs.spec.js +++ b/aio/tools/transforms/angular-api-package/processors/mergeDecoratorDocs.spec.js @@ -9,17 +9,21 @@ describe('mergeDecoratorDocs processor', () => { const injector = dgeni.configureInjector(); processor = injector.get('mergeDecoratorDocs'); + // Note that we do not include usageNotes in the tests. + processor.propertiesToMerge = ['description', 'shortDescription']; + moduleDoc = {}; decoratorDoc = { name: 'Component', docType: 'const', - description: 'A description of the metadata for the Component decorator', + shortDescription: 'decorator - short description', + description: 'decorator - description', symbol: { valueDeclaration: { initializer: { expression: { text: 'makeDecorator' }, arguments: [{ text: 'X' }] } } }, members: [ - { name: 'templateUrl', description: 'A description of the templateUrl property' } + { name: 'templateUrl', description: 'templateUrl - description' } ], moduleDoc }; @@ -27,15 +31,15 @@ describe('mergeDecoratorDocs processor', () => { metadataDoc = { name: 'ComponentDecorator', docType: 'interface', - description: 'A description of the interface for the call signature for the Component decorator', + description: 'call interface - description', members: [ { isCallMember: true, - description: 'The actual description of the call signature', - usageNotes: 'Use it like this...' + description: 'call interface - call member - description', + usageNotes: 'call interface - call member - usageNotes', }, { - description: 'Some other member' + description: 'call interface - non call member - description' } ], moduleDoc @@ -65,10 +69,13 @@ describe('mergeDecoratorDocs processor', () => { expect(decoratorDoc.decoratorType).toEqual('X'); }); - it('should copy across properties from the call signature doc', () => { + it('should copy across specified properties from the call signature doc', () => { processor.$process([decoratorDoc, metadataDoc, otherDoc]); - expect(decoratorDoc.description).toEqual('The actual description of the call signature'); - expect(decoratorDoc.usageNotes).toEqual('Use it like this...'); + expect(decoratorDoc.description).toEqual('call interface - call member - description'); + // Since usageNotes is not in `propertiesToMerge` it will not get copied over in these tests. + expect(decoratorDoc.usageNotes).toBeUndefined(); + // Since `shortDescription` does not exist on the call-member this will not get overridden. + expect(decoratorDoc.shortDescription).toEqual('decorator - short description'); }); it('should remove the metadataDoc from the module exports', () => { diff --git a/aio/tools/transforms/helpers/utils.js b/aio/tools/transforms/helpers/utils.js index 945a210eed..3ca5556beb 100644 --- a/aio/tools/transforms/helpers/utils.js +++ b/aio/tools/transforms/helpers/utils.js @@ -96,5 +96,19 @@ module.exports = { attrMap[key] === false ? '' : attrMap[key] === true ? ` ${key}` : ` ${key}="${attrMap[key].replace(/"/g, '"')}"`).join(''); - } + }, + + /** + * Merge the specified properties from the source to the target document + * @param {Document} target The document to receive the properties from the source + * @param {Document} source The document from which to get the properties to merge + * @param {string[]} properties A collection of the names of the properties to merge + */ + mergeProperties(target, source, properties) { + properties.forEach(property => { + if (source.hasOwnProperty(property)) { + target[property] = source[property]; + } + }); + }, }; diff --git a/aio/tools/transforms/helpers/utils.spec.js b/aio/tools/transforms/helpers/utils.spec.js index 952336e92d..ee086b8374 100644 --- a/aio/tools/transforms/helpers/utils.spec.js +++ b/aio/tools/transforms/helpers/utils.spec.js @@ -1,4 +1,4 @@ -const { mapObject, parseAttributes, renderAttributes } = require('./utils'); +const { mergeProperties, mapObject, parseAttributes, renderAttributes } = require('./utils'); describe('utils', () => { describe('mapObject', () => { @@ -96,4 +96,34 @@ describe('utils', () => { expect(renderAttributes({ })).toEqual(''); }); }); + + describe('mergeProperties', () => { + it('should write specified properties from the source to the target', () => { + const source = { a: 1, b: 2, c: 3 }; + const target = { }; + mergeProperties(target, source, ['a', 'b']); + expect(target).toEqual({ a: 1, b: 2 }); + }); + + it('should not overwrite target properties that are not specified', () => { + const source = { a: 1, b: 2, c: 3 }; + const target = { b: 10 }; + mergeProperties(target, source, ['a']); + expect(target).toEqual({ a: 1, b: 10 }); + }); + + it('should not overwrite target properties that are specified but do not exist in the source', () => { + const source = { a: 1 }; + const target = { b: 10 }; + mergeProperties(target, source, ['a', 'b']); + expect(target).toEqual({ a: 1, b: 10 }); + }); + + it('should overwrite target properties even if they are `undefined` in the source', () => { + const source = { a: undefined }; + const target = { a: 10 }; + mergeProperties(target, source, ['a']); + expect(target).toEqual({ a: undefined }); + }); + }); });