adopted-ember-addons / adopted-ember-addons/ember-cli-sass
includePaths is overwritten in in-repo addons when defined in parent app
- 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