-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[WIP][BUGFIX] Not overwrite the custom css configuration #5305
Conversation
this seems reasonable. cc @chadhietala i believe your change to fix the options merging may be related, can you 👀 |
@@ -202,6 +199,13 @@ EmberApp.prototype._initOptions = function(options, isProduction) { | |||
} | |||
}); | |||
|
|||
// Do not override custom css config if it was configured | |||
this.options.outputPaths.app = Object.assign({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are supporting Node 0.12.0 this will not work due to Object.assign not being a thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we also support node 0.10.x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @chadhietala, fixed
cc10f69
to
b4f501e
Compare
☔ The latest upstream changes (presumably #5367) made this pull request unmergeable. Please resolve the merge conflicts. |
this needs a rebase: @chadhietala can you take another look, |
Seems good. |
b4f501e
to
d92739e
Compare
I need to check the tests failing. |
☔ The latest upstream changes (presumably #5379) made this pull request unmergeable. Please resolve the merge conflicts. |
@@ -256,7 +256,7 @@ EmberApp.prototype._initVendorFiles = function() { | |||
handlebarsVendorFiles = null; | |||
} | |||
|
|||
this.vendorFiles = omitBy(merge({ | |||
this.vendorFiles = omit(merge({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not believe that we have omit
being required any more, I believe it should be omitBy
and this sholdn't need to be changed...
@dukex can you update based on the above feedback + rebase ? |
@stefanpenner sure, I'll do it today |
d92739e
to
65d13f7
Compare
@@ -286,7 +286,9 @@ EmberApp.prototype._initVendorFiles = function() { | |||
} | |||
} | |||
] | |||
}, this.options.vendorFiles), isNull); | |||
}, this.options.vendorFiles), function(value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not seeing how this is any different that what we already have since _.isNull
is actually the same code:
function isNull(value) {
return value === null;
}
I'm also confused how the merging of vendorFiles
is related to outputPaths
.
@dukex ping |
closing due to inactivity |
Today the
defaults
merge is overwrite the cssoutputPath
configured always adding the app config.This pull request fix #4948 and is related with adopted-ember-addons/ember-cli-sass#81
See the problem, given the follow config
With this config today it's expected a app.css but the user explicitly overwrote the css config. In the other hand the guides teach the user put the app file when he is overwrite the css outputPath
A alternative and easy fix is tell to users always have a app.css because the ember will always send to addon the
outputPaths
withapp
ps: Maybe I need fix some acceptance tests