adopted-ember-addons / adopted-ember-addons/ember-cli-sass

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

Ouverte
#182 1 commentaire 3 réactions 0 personnes assignées Voir sur GitHub
Langage dominant
JavaScript
Étoiles
273
Forks
90
Métriques de merge des PR
Aucune PR mergée en 30 j

Description

### 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](https://github.com/aexmachina/ember-cli-sass/blob/master/README.md#usage-within-in-repo-addon-and-in-repo-engine) 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`...
```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`...
```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`...
```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](https://github.com/aexmachina/ember-cli-sass/blob/master/index.js#L62), `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`...
```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!

Guide de contribution

Aucun guide de contribution indexé pour ce dépôt

Piste de recherche

Start with index.js around line 62 and inspect how sassOptions are combined between the parent app and in-repo engine. Reproduce the case using the ember-cli-build.js, lib/my-engine/index.js, and addon/styles/addon.scss examples, then run ember build. Done means both parent and engine includePaths remain available so common/vars resolves.

Rédigé par le modèle d'indexation à partir du texte de l'issue.

Évaluation

Stack technique
javascript, sass
Domaine
build-system
Type d'issue
Bug
Difficulté
3/5
Temps estimé
1-2 jours
Activité
À l'abandon
Clarté
Clairement spécifiée
Accessibilité débutants
45/100

Recevez les nouvelles issues par e-mail

Un résumé court des issues GitHub adaptées aux débutants.