includePaths is overwritten in in-repo addons when defined in parent app

[bartsqueezy]

Summary

With the increasing adoption of ember-engines, a commonly observed pattern is to create a "common" in-repo addon that exposes shared styling, variables, mixins, functions, etc to the parent application and other in-repo engines. The instructions under the usage within in-repo addon section in the README work great. However, if you have also defined sassOptions.includePaths within the parent app's ember-cli-build.js file, it currently overwrites includePaths set within any in-repo engine.

Example

ember-cli-build.js...

const EmberApp = require('ember-cli/lib/broccoli/ember-app');

module.exports = function (defaults) {
  const app = new EmberApp(defaults, {
    sassOptions: {
      includePaths: [
        'node_modules/spinkit/scss'
      ]
    }
  });

  return app.toTree();
};

lib/my-engine/index.js...

const EngineAddon = require('ember-engines/lib/engine-addon');

module.exports = EngineAddon.extend({
  name: 'my-engine',

  lazyLoading: {
    enabled: true
  },

  isDevelopingAddon() {
    return true;
  },

  sassOptions: {
    includePaths: ['lib/common/app/styles']
  }
});

lib/my-engine/addon/styles/addon.scss...

@import 'common/vars';

When running ember build, it fails with an import error...

Error: Error: File to import not found or unreadable: common/vars.

This happens because on line 62 of index.js, parentOption is being merged into options, which is overwriting the includePaths value set within the in-repo engine.

I agree that we should be merging option objects with parent apps. However, I feel it's more natural for an in-repo addon to overwrite a parent app's options, instead of the other way around. In the case of includePaths, I also think merging these two arrays would be the proper fix as well.

Proposed Solution

index.js...

sassOptions: {
  var env  = process.env.EMBER_ENV;
  var options = (this.app && this.app.options && this.app.options.sassOptions) || {};
  var parentOption = (this.parent && this.parent.app && this.parent.app.options && this.parent.app.options.sassOptions) || {};
  var envConfig = this.project.config(env).sassOptions;

  if (parentOption.includePaths) {
    options.includePaths = options.includePaths.concat(parentOption.includePaths);
  }

  Object.assign(parentOption, options);

  // ...
}

I've tested this out in a new ember application and it's solves this use case. I'm happy to submit a PR for this. Just wanted to submit the idea first to gather feedback beforehand.

Thanks for all of your time in keeping this package updated!

[GCheung55]

I'm seeing an error that seems to be related to this issue due to 18bce89.

Basically, I have an in-repo-engine that depends on ember-cli-foundation-6-sass. Due to the logic in the commit above, the includePaths defined in the in-repo-engine is overridden and I get the following error:

Sass Syntax Error (SassCompiler) in /var/folders/gx/tk9c_vbd2qggyd7b6q10qmkw0000gn/T/broccoli-718244Wte41wFNgjO/out-1823-broccoli_merge_trees/foundation-sites/foundation.scss:9:9

Error: Can't find stylesheet to import.
  ╷
9 │ @import '../_vendor/normalize-scss/sass/normalize';
  │         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  /var/folders/gx/tk9c_vbd2qggyd7b6q10qmkw0000gn/T/broccoli-718244Wte41wFNgjO/out-1823-broccoli_merge_trees/foundation-sites/foundation.scss 9:9  @import
  lib/my-engine/addon/styles/addon.scss 12:9                                                                                                root stylesheet