From 847e794c14cdeaa76054f8a0ad5b04c0f644756c Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 5 Jul 2021 13:38:23 +0200 Subject: [PATCH 1/2] Ensure app macro configs take precedence over addon configs --- packages/macros/src/ember-addon-main.ts | 15 ++++++++++-- packages/macros/src/macros-config.ts | 20 ++++++++++++++-- tests/fixtures/macro-sample-addon/index.js | 3 ++- tests/fixtures/macro-test/ember-cli-build.js | 1 + .../components/cross-package-config-test.js | 24 +++++++++++++++---- 5 files changed, 54 insertions(+), 9 deletions(-) diff --git a/packages/macros/src/ember-addon-main.ts b/packages/macros/src/ember-addon-main.ts index 05406ae4d..38e80b4d4 100644 --- a/packages/macros/src/ember-addon-main.ts +++ b/packages/macros/src/ember-addon-main.ts @@ -14,13 +14,24 @@ export = { // if parent is an addon it has root. If it's an app it has project.root. let source = parent.root || parent.project.root; + // In order to allow to overwrite config from consumed addons, we keep a priority + // This is stripped before being returned via getConfig/getOwnConfig + // Own config has lowest priority - e.g. default values for an addon + // Addons can overwrite other addon's config (priority 1) + // The host app has highest priority (priority 2) if (ownOptions.setOwnConfig) { - MacrosConfig.for(appInstance).setOwnConfig(source, ownOptions.setOwnConfig); + MacrosConfig.for(appInstance).setOwnConfig(source, Object.assign({ __priority: 0 }, ownOptions.setOwnConfig)); } if (ownOptions.setConfig) { + let parentIsHostApp = parent === appInstance; + for (let [packageName, config] of Object.entries(ownOptions.setConfig)) { - MacrosConfig.for(appInstance).setConfig(source, packageName, config); + MacrosConfig.for(appInstance).setConfig( + source, + packageName, + Object.assign({ __priority: parentIsHostApp ? 2 : 1 }, config) + ); } } diff --git a/packages/macros/src/macros-config.ts b/packages/macros/src/macros-config.ts index 275ead917..ba52c0e4d 100644 --- a/packages/macros/src/macros-config.ts +++ b/packages/macros/src/macros-config.ts @@ -210,7 +210,9 @@ export default class MacrosConfig { if (configs.length > 1) { combined = this.mergerFor(pkgRoot)(configs); } else { - combined = configs[0]; + let config = configs[0] as unknown & { __priority?: number }; + delete config.__priority; + combined = config; } userConfigs[pkgRoot] = combined; } @@ -339,5 +341,19 @@ export default class MacrosConfig { } function defaultMerger(configs: unknown[]): unknown { - return Object.assign({}, ...configs); + configs.sort((a, b) => { + let priorityA = (a as unknown & { __priority?: number }).__priority || 0; + let priorityB = (b as unknown & { __priority?: number }).__priority || 0; + + if (priorityA === priorityB) { + return 0; + } + + return priorityA > priorityB ? 1 : -1; + }); + + let mergedConfig = Object.assign({}, ...configs); + delete mergedConfig.__priority; + + return mergedConfig; } diff --git a/tests/fixtures/macro-sample-addon/index.js b/tests/fixtures/macro-sample-addon/index.js index bdd728d6e..9b631f74b 100644 --- a/tests/fixtures/macro-sample-addon/index.js +++ b/tests/fixtures/macro-sample-addon/index.js @@ -5,7 +5,8 @@ module.exports = { options: { '@embroider/macros': { setOwnConfig: { - hello: 'world', + shouldBeOverwritten: 'not overwritten', + configFromAddonItself: 'this is the addon', }, }, }, diff --git a/tests/fixtures/macro-test/ember-cli-build.js b/tests/fixtures/macro-test/ember-cli-build.js index 92f32c204..7daf1d63f 100644 --- a/tests/fixtures/macro-test/ember-cli-build.js +++ b/tests/fixtures/macro-test/ember-cli-build.js @@ -19,6 +19,7 @@ module.exports = function (defaults) { }, 'macro-sample-addon': { configFromMacrosTests: 'exists', + shouldBeOverwritten: 'overwritten', }, }, }, diff --git a/tests/fixtures/macro-test/tests/integration/components/cross-package-config-test.js b/tests/fixtures/macro-test/tests/integration/components/cross-package-config-test.js index 8b4740ec0..7fad9f6b5 100644 --- a/tests/fixtures/macro-test/tests/integration/components/cross-package-config-test.js +++ b/tests/fixtures/macro-test/tests/integration/components/cross-package-config-test.js @@ -13,14 +13,22 @@ module('Integration | cross-package-config', function (hooks) { this.owner.register( 'helper:my-assertion', helper(function ([value]) { - assert.deepEqual(value, { hello: 'world', configFromMacrosTests: 'exists' }); + assert.deepEqual(value, { + shouldBeOverwritten: 'overwritten', + configFromAddonItself: 'this is the addon', + configFromMacrosTests: 'exists', + }); }) ); await render(hbs`{{my-assertion (reflect-config)}}`); }); test(`app's JS can see addon's merged config`, async function (assert) { - assert.deepEqual(reflectAddonConfig(), { hello: 'world', configFromMacrosTests: 'exists' }); + assert.deepEqual(reflectAddonConfig(), { + shouldBeOverwritten: 'overwritten', + configFromAddonItself: 'this is the addon', + configFromMacrosTests: 'exists', + }); }); test(`addon's HBS can see addon's merged config`, async function (assert) { @@ -28,7 +36,11 @@ module('Integration | cross-package-config', function (hooks) { this.owner.register( 'helper:my-assertion', helper(function ([value]) { - assert.deepEqual(value, { hello: 'world', configFromMacrosTests: 'exists' }); + assert.deepEqual(value, { + shouldBeOverwritten: 'overwritten', + configFromAddonItself: 'this is the addon', + configFromMacrosTests: 'exists', + }); }) ); await render(hbs`{{#reflect-hbs-config as |config|}} {{my-assertion config}} {{/reflect-hbs-config}}`); @@ -39,7 +51,11 @@ module('Integration | cross-package-config', function (hooks) { this.owner.register( 'helper:my-assertion', helper(function ([value]) { - assert.deepEqual(value, { hello: 'world', configFromMacrosTests: 'exists' }); + assert.deepEqual(value, { + shouldBeOverwritten: 'overwritten', + configFromAddonItself: 'this is the addon', + configFromMacrosTests: 'exists', + }); }) ); await render(hbs`{{my-assertion (macroGetConfig "macro-sample-addon" )}}`); From 4e7e7b635859712c4049591735b960ed7797cb68 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Mon, 5 Jul 2021 11:32:32 -0400 Subject: [PATCH 2/2] expose sourceOfConfig to mergers --- packages/macros/src/ember-addon-main.ts | 15 +--- packages/macros/src/macros-config.ts | 98 +++++++++++++++++-------- 2 files changed, 70 insertions(+), 43 deletions(-) diff --git a/packages/macros/src/ember-addon-main.ts b/packages/macros/src/ember-addon-main.ts index 38e80b4d4..bdd019146 100644 --- a/packages/macros/src/ember-addon-main.ts +++ b/packages/macros/src/ember-addon-main.ts @@ -14,24 +14,13 @@ export = { // if parent is an addon it has root. If it's an app it has project.root. let source = parent.root || parent.project.root; - // In order to allow to overwrite config from consumed addons, we keep a priority - // This is stripped before being returned via getConfig/getOwnConfig - // Own config has lowest priority - e.g. default values for an addon - // Addons can overwrite other addon's config (priority 1) - // The host app has highest priority (priority 2) if (ownOptions.setOwnConfig) { - MacrosConfig.for(appInstance).setOwnConfig(source, Object.assign({ __priority: 0 }, ownOptions.setOwnConfig)); + MacrosConfig.for(appInstance).setOwnConfig(source, ownOptions.setOwnConfig); } if (ownOptions.setConfig) { - let parentIsHostApp = parent === appInstance; - for (let [packageName, config] of Object.entries(ownOptions.setConfig)) { - MacrosConfig.for(appInstance).setConfig( - source, - packageName, - Object.assign({ __priority: parentIsHostApp ? 2 : 1 }, config) - ); + MacrosConfig.for(appInstance).setConfig(source, packageName, config as object); } } diff --git a/packages/macros/src/macros-config.ts b/packages/macros/src/macros-config.ts index ba52c0e4d..b121390d2 100644 --- a/packages/macros/src/macros-config.ts +++ b/packages/macros/src/macros-config.ts @@ -3,17 +3,30 @@ import type { PluginItem } from '@babel/core'; import { PackageCache, getOrCreate } from '@embroider/shared-internals'; import { makeFirstTransform, makeSecondTransform } from './glimmer/ast-transform'; import State from './babel/state'; +import partition from 'lodash/partition'; const packageCache = new PackageCache(); -export type Merger = (configs: unknown[]) => unknown; +export type SourceOfConfig = (config: object) => { + readonly name: string; + readonly root: string; + readonly version: string; +}; + +export type Merger = ( + configs: object[], + params: { + sourceOfConfig: SourceOfConfig; + } +) => object; // Do not change this type signature without pondering deeply the mysteries of // being compatible with unwritten future versions of this library. type GlobalSharedState = WeakMap< any, { - configs: Map; + configs: Map; + configSources: WeakMap; mergers: Map; } >; @@ -38,9 +51,16 @@ export default class MacrosConfig { } let shared = g.__embroider_macros_global__.get(key); - if (!shared) { + if (shared) { + // if an earlier version of @embroider/macros created the shared state, it + // would have configSources. + if (!shared.configSources) { + shared.configSources = new WeakMap(); + } + } else { shared = { configs: new Map(), + configSources: new WeakMap(), mergers: new Map(), }; g.__embroider_macros_global__.set(key, shared); @@ -48,6 +68,7 @@ export default class MacrosConfig { let config = new MacrosConfig(); config.configs = shared.configs; + config.configSources = shared.configSources; config.mergers = shared.mergers; localSharedState.set(key, config); return config; @@ -132,20 +153,21 @@ export default class MacrosConfig { } private _configWritable = true; - private configs: Map = new Map(); + private configs: Map = new Map(); + private configSources: WeakMap = new WeakMap(); private mergers: Map = new Map(); // Registers a new source of configuration to be given to the named package. // Your config type must be json-serializable. You must always set fromPath to // `__filename`. - setConfig(fromPath: string, packageName: string, config: unknown) { + setConfig(fromPath: string, packageName: string, config: object) { return this.internalSetConfig(fromPath, packageName, config); } // Registers a new source of configuration to be given to your own package. // Your config type must be json-serializable. You must always set fromPath to // `__filename`. - setOwnConfig(fromPath: string, config: unknown) { + setOwnConfig(fromPath: string, config: object) { return this.internalSetConfig(fromPath, undefined, config); } @@ -157,7 +179,7 @@ export default class MacrosConfig { // // Your value must be json-serializable. You must always set fromPath to // `__filename`. - setGlobalConfig(fromPath: string, key: string, value: unknown) { + setGlobalConfig(fromPath: string, key: string, value: object) { if (!this._configWritable) { throw new Error( `[Embroider:MacrosConfig] attempted to set global config after configs have been finalized from: '${fromPath}'` @@ -166,7 +188,7 @@ export default class MacrosConfig { this.globalConfig[key] = value; } - private internalSetConfig(fromPath: string, packageName: string | undefined, config: unknown) { + private internalSetConfig(fromPath: string, packageName: string | undefined, config: object) { if (!this._configWritable) { throw new Error( `[Embroider:MacrosConfig] attempted to set config after configs have been finalized from: '${fromPath}'` @@ -176,6 +198,7 @@ export default class MacrosConfig { let targetPackage = this.resolvePackage(fromPath, packageName); let peers = getOrCreate(this.configs, targetPackage.root, () => []); peers.push(config); + this.configSources.set(config, fromPath); } // Allows you to set the merging strategy used for your package's config. The @@ -196,7 +219,7 @@ export default class MacrosConfig { this.mergers.set(targetPackage.root, { merger, fromPath }); } - private cachedUserConfigs: { [packageRoot: string]: unknown } | undefined; + private cachedUserConfigs: { [packageRoot: string]: object } | undefined; private get userConfigs() { if (this._configWritable) { @@ -204,15 +227,14 @@ export default class MacrosConfig { } if (!this.cachedUserConfigs) { - let userConfigs: { [packageRoot: string]: unknown } = {}; + let userConfigs: { [packageRoot: string]: object } = {}; + let sourceOfConfig = makeConfigSourcer(this.configSources); for (let [pkgRoot, configs] of this.configs) { - let combined: unknown; + let combined: object; if (configs.length > 1) { - combined = this.mergerFor(pkgRoot)(configs); + combined = this.mergerFor(pkgRoot)(configs, { sourceOfConfig }); } else { - let config = configs[0] as unknown & { __priority?: number }; - delete config.__priority; - combined = config; + combined = configs[0]; } userConfigs[pkgRoot] = combined; } @@ -300,7 +322,7 @@ export default class MacrosConfig { if (entry) { return entry.merger; } - return defaultMerger; + return defaultMergerFor(pkgRoot); } // this exists because @embroider/compat rewrites and moves v1 addons, and @@ -340,20 +362,36 @@ export default class MacrosConfig { } } -function defaultMerger(configs: unknown[]): unknown { - configs.sort((a, b) => { - let priorityA = (a as unknown & { __priority?: number }).__priority || 0; - let priorityB = (b as unknown & { __priority?: number }).__priority || 0; +function defaultMergerFor(pkgRoot: string) { + return function defaultMerger(configs: object[], { sourceOfConfig }: { sourceOfConfig: SourceOfConfig }): object { + let [ownConfigs, otherConfigs] = partition(configs, c => sourceOfConfig(c as object).root === pkgRoot); + return Object.assign({}, ...ownConfigs, ...otherConfigs); + }; +} - if (priorityA === priorityB) { - return 0; +function makeConfigSourcer(configSources: WeakMap): SourceOfConfig { + return config => { + let fromPath = configSources.get(config); + if (!fromPath) { + throw new Error(`unknown object passed to sourceOfConfig(). You can only pass back the configs you were given.`); } - - return priorityA > priorityB ? 1 : -1; - }); - - let mergedConfig = Object.assign({}, ...configs); - delete mergedConfig.__priority; - - return mergedConfig; + let maybePkg = packageCache.ownerOfFile(fromPath); + if (!maybePkg) { + throw new Error( + `bug: unexpected error, we always check that fromPath is owned during internalSetConfig so this should never happen` + ); + } + let pkg = maybePkg; + return { + get name() { + return pkg.name; + }, + get version() { + return pkg.version; + }, + get root() { + return pkg.root; + }, + }; + }; }