Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addons can provide their resolver module configuration via resolverConfig() #348

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 62 additions & 1 deletion index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';

var writeFile = require('broccoli-file-creator');
var VersionChecker = require('ember-cli-version-checker');
var path = require('path');
var isModuleUnification;
Expand Down Expand Up @@ -57,7 +58,61 @@ module.exports = {
return new MergeTrees(addonTrees);
},

_moduleUnificationTrees() {
// Trigger exception if the result of `addon.resolverConfig` method has an unexpected format
// Show a warning if there are collisions between addon types or addon collections
validateAddonsConfig: function(addonsConfig) {

let types = {};
let collections = {};

Object.keys(addonsConfig).forEach(addonName => {

let addonConfig = addonsConfig[addonName];
if (addonConfig) {
if (typeof(addonConfig) !== 'object') {
throw new Error(`"addon.resolverConfig" returns an unexpected value. Addon: ${addonName}.`);
ppcano marked this conversation as resolved.
Show resolved Hide resolved
ppcano marked this conversation as resolved.
Show resolved Hide resolved
}

let addonTypes = addonConfig.types || {};
if (typeof(addonTypes) !== 'object') {
throw new Error(`"addon.resolverConfig" returns an unexpected "types" value. Addon: ${addonName}.`);
}

let addonCollections = addonConfig.collections || {};
if (typeof(addonCollections) !== 'object') {
throw new Error(`"addon.resolverConfig" returns an unexpected "collections" value. Addon: ${addonName}.`);
}

Object.keys(addonTypes).forEach(key => {
if (!types.hasOwnProperty(key)) {
types[key] = key;
} else {
this.ui.writeLine(`Addon '${types[key]}' configured the type '${key}' on the resolver, but addon '${addonName}' has overwritten the type '${key}'.`);
ppcano marked this conversation as resolved.
Show resolved Hide resolved
}
});
Object.keys(addonCollections).forEach(key => {
if (!collections.hasOwnProperty(key)) {
collections[key] = key;
} else {
this.ui.writeLine(`Addon '${types[key]}' configured the collection '${key}' on the resolver, but addon '${addonName}' has overwritten the collection '${key}'.`);
}
});
}
});
},

_moduleUnificationTrees: function() {

let addonConfigs = {};
this.project.addons.forEach(addon => {
if (addon.resolverConfig) {
addonConfigs[addon.name] = addon.resolverConfig() || {};
}
});
this.validateAddonsConfig(addonConfigs);

let addonConfigsFileContent = `export default ${JSON.stringify(addonConfigs)};`;

var resolve = require('resolve');
var Funnel = require('broccoli-funnel');

Expand All @@ -66,6 +121,11 @@ module.exports = {
destDir: 'ember-resolver'
});

var addonsConfigTree = writeFile(
'ember-resolver/addons-config.js',
addonConfigsFileContent
);

var glimmerResolverSrc = require.resolve('@glimmer/resolver/package');
var glimmerResolverPath = path.dirname(glimmerResolverSrc);
var glimmerResolverTree = new Funnel(glimmerResolverPath, {
Expand All @@ -80,6 +140,7 @@ module.exports = {
});

return [
this.preprocessJs(addonsConfigTree, { registry: this.registry }),
this.preprocessJs(featureTree, { registry: this.registry }),
this.preprocessJs(glimmerResolverTree, { registry: this.registry }),
this.preprocessJs(glimmerDITree, { registry: this.registry }),
Expand Down
67 changes: 67 additions & 0 deletions mu-trees/addon/merge-addons-config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand why this is included in addon/ and not done completely in index.js, what am I missing?

Copy link
Contributor Author

@ppcano ppcano Mar 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwjblue What is your proposal in this case? Let's see if I can understand your suggestion.

In this proposal, the resolver looks like:

     import Resolver from 'ember-resolver/resolvers/fallback';
     import buildResolverConfig from 'ember-resolver/ember-config';
     import config from '../config/environment';
     import addonsConfig from 'ember-resolver/addons-config';
     import mergeAddonsConfig from 'ember-resolver/merge-addons-config';

     let moduleConfig = buildResolverConfig(config.modulePrefix);
     mergeAddonsConfig(moduleConfig, addonsConfig);

     export default Resolver.extend({
       config: moduleConfig
     });

The only benefit is that users can modify the addonsConfig to skip the addon collision exception.

Are you suggesting to perform the addon config merge within the buildResolverConfig function and continue using the current MU resolver.js like:

     import Resolver from 'ember-resolver/resolvers/fallback';
     import buildResolverConfig from 'ember-resolver/ember-config';
     import config from '../config/environment';
     

     let moduleConfig = buildResolverConfig(config.modulePrefix);

     export default Resolver.extend({
       config: moduleConfig
     });

or something else?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm suggesting to do the final merging in node land and using broccoli-file-creator to write the final config to ember-resolver/ember-config directly.

Copy link
Contributor Author

@ppcano ppcano Mar 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwjblue

I think autogenerating the whole ember-resolver/ember-config is not ideal, because this is a function that needs the config.modulePrefix.

I have changed my implementation to execute mergeAddonsConfig within the ember-resolver/ember-config which has been refactored:

import addonsConfig from 'ember-resolver/addons-config';
import moduleConfig from 'ember-resolver/module-config';
import mergeAddonsConfig from 'ember-resolver/utils/merge-addons-config';

export default function generateConfig(name) {

  let config = {
    app: {
      name,
      rootName: name
    },
  };

  mergeAddonsConfig(moduleConfig, addonsConfig);

  return Object.assign(config, moduleConfig);
}

This setup allows using the same default src/resolver.js as today.

     import Resolver from 'ember-resolver/resolvers/fallback';
     import buildResolverConfig from 'ember-resolver/ember-config';
     import config from '../config/environment';
     
     let moduleConfig = buildResolverConfig(config.modulePrefix);

     export default Resolver.extend({
       config: moduleConfig
     });

Done at 4d29950

This function merges the types and collections from addons `addonsConfig`
into the `config` parameter.

It will throw an exception if an addon tries to override
an existing type or collection on the resolver config object or
if two addons provide the same type or collection.

- `config`: is the resolver config generated with `ember-resolver/ember-config`

- `addonsConfig`: is a hash object containing the result of the call of
`addon.resolverConfig` method on all the project addons.

```js
{
my-addon1: { types: { ... } },
my-addon2: { collections: { ... } },
hola-addon: { types: { ... }, collections: { ... } }
}
```

The value of `addonsConfig` can be imported from 'ember-resolver/addons-config'.

Usage:

```resolver.js
import Resolver from 'ember-resolver/resolvers/fallback';
import buildResolverConfig from 'ember-resolver/ember-config';
import config from '../config/environment';
import addonsConfig from 'ember-resolver/addons-config';
import mergeAddonsConfig from 'ember-resolver/merge-addons-config';

let moduleConfig = buildResolverConfig(config.modulePrefix);
mergeAddonsConfig(moduleConfig, addonsConfig);

export default Resolver.extend({
config: moduleConfig
});
```
*/
export default function mergeAddonsConfig(config, addonsConfig) {

// This implementation does not allow an addon to overwrite the default MU module resolution.
// Is this the expected behaviour?
ppcano marked this conversation as resolved.
Show resolved Hide resolved
Object.keys(addonsConfig).forEach(function (addonName) {
let addonConfig = addonsConfig[addonName];
let addonTypes = addonConfig.types || {};
let addonCollections = addonConfig.collections || {};

Object.keys(addonTypes).forEach(function (key) {
if (!config.types.hasOwnProperty(key)) {
config.types[key] = addonTypes[key];
} else {
// A similar validation is done during the build phase on `index.validateAddonsConfig`
throw new Error(`Addon '${addonName}' attempts to configure the type '${key}' on the resolver but '${key}' has already been configured.`);
}
});
Object.keys(addonCollections).forEach(function (key) {
if (!config.collections.hasOwnProperty(key)) {
config.collections[key] = addonCollections[key];
} else {
// A similar validation is done during the build phase on `index.validateAddonsConfig`
throw new Error(`Addon '${addonName}' attempts to configure the collection '${key}' on the resolver but '${key}' has already been configured.`);
}
});
});
}
74 changes: 74 additions & 0 deletions mu-trees/tests/unit/merge-addons-config-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
import { module, test } from "qunit";
import mergeAddonsConfig from "ember-resolver/merge-addons-config";

module("ember-resolver/merge-addons-config");

const emptyConfig = function() {
return { types: {}, collections: {} };
};

test("Trigger error if two addons configure the same type", function(assert) {
ppcano marked this conversation as resolved.
Show resolved Hide resolved
let addonsConfig = {
"my-addon1": { types: { hola: "hola" } },
"my-addon2": { types: { hola: "adios" } }
};

assert.throws(function() {
mergeAddonsConfig(emptyConfig(), addonsConfig);
});
});

test("Trigger error if two addons configure the same collection", function(assert) {
let addonsConfig = {
"my-addon1": { collections: { hola: "hola" } },
"my-addon2": { collections: { hola: "adios" } }
};

assert.throws(function() {
mergeAddonsConfig(emptyConfig(), addonsConfig);
});
});

test("Trigger error if addon overwrite an existing type", function(assert) {
let addonsConfig = {
"my-addon1": { types: { hola: "new-hola" } }
};

let config = { types: { hola: "hola" } };
assert.throws(function() {
mergeAddonsConfig(config, addonsConfig);
});
});

test("Trigger error if addon overwrite an existing collection", function(assert) {
let addonsConfig = {
"my-addon1": { collections: { hola: "new-hola" } }
};

let config = { collections: { hola: "hola" } };
assert.throws(function() {
mergeAddonsConfig(config, addonsConfig);
});
});

test("Can merge collections", function(assert) {
let addonsConfig = {
"my-addon1": { collections: { col1: "foo" } },
"my-addon2": { collections: { col2: "baz" } }
};

let result = emptyConfig();
mergeAddonsConfig(result, addonsConfig);
assert.deepEqual(result.collections, { col1: "foo", col2: "baz" });
});

test("Can merge types", function(assert) {
let addonsConfig = {
"my-addon1": { types: { col1: "foo" } },
"my-addon2": { types: { col2: "baz" } }
};

let result = emptyConfig();
mergeAddonsConfig(result, addonsConfig);
assert.deepEqual(result.types, { col1: "foo", col2: "baz" });
});
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
"dependencies": {
"@glimmer/resolver": "^0.4.1",
"babel-plugin-debug-macros": "^0.1.10",
"broccoli-file-creator": "^2.1.1",
"broccoli-funnel": "^2.0.2",
"broccoli-merge-trees": "^3.0.0",
"ember-cli-babel": "^6.16.0",
Expand Down
11 changes: 10 additions & 1 deletion yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1542,6 +1542,14 @@ broccoli-debug@^0.6.4, broccoli-debug@^0.6.5:
symlink-or-copy "^1.1.8"
tree-sync "^1.2.2"

broccoli-file-creator@^2.1.1:
version "2.1.1"
resolved "https://registry.yarnpkg.com/broccoli-file-creator/-/broccoli-file-creator-2.1.1.tgz#7351dd2496c762cfce7736ce9b49e3fce0c7b7db"
integrity sha512-YpjOExWr92C5vhnK0kmD81kM7U09kdIRZk9w4ZDCDHuHXW+VE/x6AGEOQQW3loBQQ6Jk+k+TSm8dESy4uZsnjw==
dependencies:
broccoli-plugin "^1.1.0"
mkdirp "^0.5.1"

broccoli-filter@^1.0.1:
version "1.3.0"
resolved "https://registry.yarnpkg.com/broccoli-filter/-/broccoli-filter-1.3.0.tgz#71e3a8e32a17f309e12261919c5b1006d6766de6"
Expand Down Expand Up @@ -1695,9 +1703,10 @@ broccoli-persistent-filter@^2.1.1:
symlink-or-copy "^1.0.1"
walk-sync "^0.3.1"

broccoli-plugin@^1.0.0, broccoli-plugin@^1.2.0, broccoli-plugin@^1.2.1, broccoli-plugin@^1.3.0, broccoli-plugin@^1.3.1:
broccoli-plugin@^1.0.0, broccoli-plugin@^1.1.0, broccoli-plugin@^1.2.0, broccoli-plugin@^1.2.1, broccoli-plugin@^1.3.0, broccoli-plugin@^1.3.1:
version "1.3.1"
resolved "https://registry.yarnpkg.com/broccoli-plugin/-/broccoli-plugin-1.3.1.tgz#a26315732fb99ed2d9fb58f12a1e14e986b4fabd"
integrity sha512-DW8XASZkmorp+q7J4EeDEZz+LoyKLAd2XZULXyD9l4m9/hAKV3vjHmB1kiUshcWAYMgTP1m2i4NnqCE/23h6AQ==
dependencies:
promise-map-series "^0.2.1"
quick-temp "^0.1.3"
Expand Down