From 9d8885d48f06cd8486a85eeee2d86e2ae885ee66 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Wed, 7 Jan 2015 16:39:57 -0800 Subject: [PATCH 1/3] [Framework] Add extension sorter Add priority ordering to loaded extensions in each category; this allows control over the resulting order of extensions acquired and used within the application. WTD-590 --- platform/framework/src/Constants.js | 10 +- platform/framework/src/Main.js | 34 ++++--- .../src/register/ExtensionRegistrar.js | 6 +- .../framework/src/register/ExtensionSorter.js | 97 +++++++++++++++++++ 4 files changed, 129 insertions(+), 18 deletions(-) create mode 100644 platform/framework/src/register/ExtensionSorter.js diff --git a/platform/framework/src/Constants.js b/platform/framework/src/Constants.js index 6202495645..13eb346448 100644 --- a/platform/framework/src/Constants.js +++ b/platform/framework/src/Constants.js @@ -16,5 +16,13 @@ define({ "tests": "test", "configuration": {}, "extensions": {} - } + }, + PRIORITY_LEVELS: { + "fallback": Number.NEGATIVE_INFINITY, + "default": 100, + "optional": 200, + "preferred": 400, + "mandatory": Number.POSITIVE_INFINITY + }, + DEFAULT_PRIORITY: 0 }); \ No newline at end of file diff --git a/platform/framework/src/Main.js b/platform/framework/src/Main.js index c1ea28a5ac..d0c7978379 100644 --- a/platform/framework/src/Main.js +++ b/platform/framework/src/Main.js @@ -26,23 +26,26 @@ define( './resolve/RequireConfigurator', './register/CustomRegistrars', './register/ExtensionRegistrar', + './register/ExtensionSorter', './bootstrap/ApplicationBootstrapper' ], - function (require, - es6promise, - angular, - angularRoute, - Constants, - FrameworkInitializer, - BundleLoader, - ImplementationLoader, - ExtensionResolver, - BundleResolver, - RequireConfigurator, - CustomRegistrars, - ExtensionRegistrar, - ApplicationBootstrapper - ) { + function ( + require, + es6promise, + angular, + angularRoute, + Constants, + FrameworkInitializer, + BundleLoader, + ImplementationLoader, + ExtensionResolver, + BundleResolver, + RequireConfigurator, + CustomRegistrars, + ExtensionRegistrar, + ExtensionSorter, + ApplicationBootstrapper + ) { "use strict"; // Get a reference to Angular's injector, so we can get $http and $log @@ -68,6 +71,7 @@ define( registrar = new ExtensionRegistrar( app, new CustomRegistrars(app, $log), + new ExtensionSorter($log), $log ), bootstrapper = new ApplicationBootstrapper( diff --git a/platform/framework/src/register/ExtensionRegistrar.js b/platform/framework/src/register/ExtensionRegistrar.js index 2ca1b9feca..138659a12f 100644 --- a/platform/framework/src/register/ExtensionRegistrar.js +++ b/platform/framework/src/register/ExtensionRegistrar.js @@ -17,9 +17,11 @@ define( * @param {Object.} customRegistrars an object * containing custom registration functions, primarily for * Angular built-ins. + * @param {ExtensionSorter} sorter the sorter which will impose + * priority ordering upon extensions * @param {*} $log Angular's logging service */ - function ExtensionRegistrar(app, customRegistrars, $log) { + function ExtensionRegistrar(app, customRegistrars, sorter, $log) { // Track which extension categories have already been registered. // Exceptions will be thrown if the same extension category is // registered twice. @@ -163,7 +165,7 @@ define( Object.keys(extensionGroup).forEach(function (category) { registerExtensionsForCategory( category, - extensionGroup[category] + sorter.sort(extensionGroup[category]) ); }); diff --git a/platform/framework/src/register/ExtensionSorter.js b/platform/framework/src/register/ExtensionSorter.js new file mode 100644 index 0000000000..cfef4d5af1 --- /dev/null +++ b/platform/framework/src/register/ExtensionSorter.js @@ -0,0 +1,97 @@ +/*global define*/ + +define( + ["../Constants"], + function (Constants) { + "use strict"; + + /** + * Responsible for applying priority order to extensions in a + * given category. This will sort in reverse order of the numeric + * priority given for extensions in the `priority` priority (such + * that large values are registered first.) Extensions may also + * specify symbolic properties as strings (instead of numbers), + * which will be looked up from the table `Constants.PRIORITY_LEVELS`. + * @param $log Angular's logging service + * @constructor + */ + function ExtensionSorter($log) { + + // Handle unknown or malformed priorities specified by extensions + function unrecognizedPriority(extension) { + // Issue a warning + $log.warn([ + "Unrecognized priority '", + (extension || {}).priority, + "' specified for extension from ", + ((extension || {}).bundle || {}).path, + "; defaulting to ", + Constants.DEFAULT_PRIORITY + ].join('')); + + // Provide a return value (default priority) to make this + // useful in an expression. + return Constants.DEFAULT_PRIORITY; + } + + function getPriority(extension) { + var priority = + (extension || {}).priority || Constants.DEFAULT_PRIORITY; + + // If it's a symbolic priority, look it up + if (typeof priority === 'string') { + priority = Constants.PRIORITY_LEVELS[priority]; + } + + // Should be a number; otherwise, issue a warning and + // fall back to default priority level. + return (typeof priority === 'number') ? + priority : unrecognizedPriority(extension); + } + + // Attach a numeric priority to an extension; this is done in + // one pass outside of the comparator, mainly because getPriority + // may log warnings, and we only want this to happen once + // (instead of the many times that might occur during a sort.) + function prioritize(extension, index) { + return { + // The extension itself, for later unwrapping + extension: extension, + // The index, to provide a stable sort (see compare) + index: index, + // The numeric priority of the extension + priority: getPriority(extension) + }; + } + + // Unwrap the original extension + // (for use after ordering has been applied) + function deprioritize(prioritized) { + return prioritized.extension; + } + + // Compare two prioritized extensions + function compare(a, b) { + // Reverse order by numeric priority; or, original order. + return (b.priority - a.priority) || (a.index - b.index); + } + + return { + /** + * Sort extensions according to priority. + * + * @param {object[]} extensions array of resolved extensions + * @returns {object[]} the same extensions, in priority order + */ + sort: function (extensions) { + return (extensions || []) + .map(prioritize) + .sort(compare) + .map(deprioritize); + } + }; + } + + return ExtensionSorter; + } +); \ No newline at end of file From 64ede1e917f0e888af7f7a5dd1d3b782a88b2345 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Wed, 7 Jan 2015 17:06:09 -0800 Subject: [PATCH 2/3] [Framework] Add/update tests for sorting Add/update framework tests to include the sorting of extensions by priority, WTD-590. --- .../test/register/ExtensionRegistrarSpec.js | 29 ++++++++++- .../test/register/ExtensionSorterSpec.js | 50 +++++++++++++++++++ platform/framework/test/suite.json | 1 + 3 files changed, 79 insertions(+), 1 deletion(-) create mode 100644 platform/framework/test/register/ExtensionSorterSpec.js diff --git a/platform/framework/test/register/ExtensionRegistrarSpec.js b/platform/framework/test/register/ExtensionRegistrarSpec.js index 70811c09f4..c89417da4b 100644 --- a/platform/framework/test/register/ExtensionRegistrarSpec.js +++ b/platform/framework/test/register/ExtensionRegistrarSpec.js @@ -11,14 +11,24 @@ define( describe("The extension registrar", function () { var mockApp, mockLog, + mockSorter, customRegistrars, registrar; beforeEach(function () { mockApp = jasmine.createSpyObj("app", ["factory"]); mockLog = jasmine.createSpyObj("$log", ["error", "warn", "debug", "info"]); + mockSorter = jasmine.createSpyObj("sorter", ["sort"]); customRegistrars = {}; - registrar = new ExtensionRegistrar(mockApp, customRegistrars, mockLog); + + mockSorter.sort.andCallFake(function (v) { return v; }); + + registrar = new ExtensionRegistrar( + mockApp, + customRegistrars, + mockSorter, + mockLog + ); }); it("registers extensions using the factory", function () { @@ -64,6 +74,23 @@ define( expect(customRegistrars.things).toHaveBeenCalled(); }); + it("sorts extensions before registering", function () { + // Some extension definitions to sort + var a = { a: 'a' }, b = { b: 'b' }, c = { c: 'c' }; + + // Fake sorting; just reverse the array + mockSorter.sort.andCallFake(function (v) { return v.reverse(); }); + + // Register the extensions + registrar.registerExtensions({ things: [ a, b, c ] }); + + // Verify registration interactions occurred in reverse-order + [ c, b, a ].forEach(function (extension, index) { + expect(mockApp.factory.calls[index].args[1][0]()) + .toEqual(extension); + }); + }); + }); } ); \ No newline at end of file diff --git a/platform/framework/test/register/ExtensionSorterSpec.js b/platform/framework/test/register/ExtensionSorterSpec.js new file mode 100644 index 0000000000..a51ece21eb --- /dev/null +++ b/platform/framework/test/register/ExtensionSorterSpec.js @@ -0,0 +1,50 @@ +/*global define,Promise,describe,it,expect,beforeEach,jasmine,waitsFor*/ + +define( + ["../../src/register/ExtensionSorter"], + function (ExtensionSorter) { + "use strict"; + + describe("The extension sorter", function () { + var mockLog, + sorter; + + beforeEach(function () { + mockLog = jasmine.createSpyObj( + "$log", + ["error", "warn", "debug", "info"] + ); + + sorter = new ExtensionSorter(mockLog); + }); + + it("sorts extensions in priority order", function () { + var a = { priority: 10 }, + b = {}, + c = { priority: 'mandatory' }; // Should be +Inf + expect(sorter.sort([a, b, c])).toEqual([c, a, b]); + }); + + it("warns about unrecognized priorities", function () { + var a = { priority: 10 }, + b = {}, + c = { priority: 'mandatory' }, // Should be +Inf + d = { priority: 'GARBAGE-TEXT' }, + e = { priority: { mal: "formed"} }, + f = { priority: 3 }; + + // Sorting should use default order (note we assume + // a stable sort here as well) + expect(sorter.sort( + [a, b, c, d, e, f] + )).toEqual( + [c, a, f, b, d, e] + ); + + // Should have been warned exactly twice (for d & e) + expect(mockLog.warn.calls.length).toEqual(2); + }); + + }); + } +); diff --git a/platform/framework/test/suite.json b/platform/framework/test/suite.json index a46a4ecfe7..93504f01af 100644 --- a/platform/framework/test/suite.json +++ b/platform/framework/test/suite.json @@ -6,6 +6,7 @@ "load/Extension", "register/CustomRegistrars", "register/ExtensionRegistrar", + "register/ExtensionSorter", "register/PartialConstructor", "register/ServiceCompositor", "resolve/BundleResolver", From 731c2b6c2180f7a33210dfbeeb4b7c47219da400 Mon Sep 17 00:00:00 2001 From: Victor Woeltjen Date: Wed, 7 Jan 2015 17:18:39 -0800 Subject: [PATCH 3/3] [Framework] Document extension ordering rules Add a description of priority-ordering rules (implemented for WTD-590) to framework documentation. --- platform/framework/README.md | 31 +++++++++++++++++++++++++++++ platform/framework/src/Constants.js | 7 ++++--- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/platform/framework/README.md b/platform/framework/README.md index 0089ede522..f359a12f12 100644 --- a/platform/framework/README.md +++ b/platform/framework/README.md @@ -106,6 +106,37 @@ support for the category of extension being registered. list, with Angular-level dependencies are declared, and the full set is exposed as a single Angular "service." +### Priority order + +Within each category, registration occurs in priority order. An extension's +priority may be specified as a `priority` property in its extension +definition; this may be a number, or a symbolic string. Extensions are +registered in reverse numeric order (highest-priority first), and symbolic +strings are mapped to the numeric values as follows: + +* `fallback`: Negative infinity. Used for extensions that are not intended + for use (that is, they are meant to be overridden) but are present as an + option of last resort. +* `default`: -100. Used for extensions that are expected to be overridden, but + need a useful default. +* `none`: 0. Also used if no priority is specified, or if an unknown or + malformed priority is specified. +* `optional`: 100. Used for extensions that are meant to be used, but may be + overridden. +* `preferred`: 1000. Used for extensions that are specifically intended to + be used, but still may be overridden in principle. +* `mandatory`: Positive infinity. Used when an extension should definitely + not be overridden. + +These symbolic names are chosen to reflect usage where many extensions may +satisfy a given usage, but only one may be used; in this case, as a +convention it should be the lowest-ordered (highest-priority) extensions +available. In other cases, a full set (or multi-element subset) of +extensions may be desired, with a specific ordering; in these cases, it +is preferable to specify priority numerically when declaring extensions, +and to understand that extensions will be sorted according to these +conventions when using them. + ### Composite services Composite services are assumed to follow a provider-aggregator-decorator diff --git a/platform/framework/src/Constants.js b/platform/framework/src/Constants.js index 13eb346448..4ac8941ee0 100644 --- a/platform/framework/src/Constants.js +++ b/platform/framework/src/Constants.js @@ -19,9 +19,10 @@ define({ }, PRIORITY_LEVELS: { "fallback": Number.NEGATIVE_INFINITY, - "default": 100, - "optional": 200, - "preferred": 400, + "default": -100, + "none": 0, + "optional": 100, + "preferred": 1000, "mandatory": Number.POSITIVE_INFINITY }, DEFAULT_PRIORITY: 0