Don't process CSS imports by default

[wagenet]

This is a potentially dangerous default in clean-css. Ideally they'd change it there, but it doesn't seem that it's likely. The benefits of importing files by URL is also greatly reduced in ember-cli apps. See clean-css/clean-css#767

[rwjblue]

Seems good to me.

[stefanpenner]

seems like a breaking change are there somespecial steps we need to take?

[Turbo87]

@wagenet could you fix the broken test?

[wagenet]

Its not a breaking change because old versions of the minifier didn't do stuff with imports. I'll look into the tests.

On May 2, 2016, at 1:10 PM, Stefan Penner notifications@github.com wrote:

seems like a breaking change are there somespecial steps we need to take?

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

[stefanpenner]

@wagenet but now there is an interim time were they are process? How big is that window (i would like to write this off as a non-breaking change, just want to make sure we have all the facts).

[homu]

☔ The latest upstream changes (presumably #5928) made this pull request unmergeable. Please resolve the merge conflicts.

[wagenet]

@homu I've fixed the merge conflict.
@stefanpenner Failing to pre-import http imports shouldn't break anything, it just could make things a bit slower at build time. Given that doing the import can break things this seems like a worthwhile trade-off, especially given that this was the previous behavior.

[nathanhammond]

@wagenet How long was the timeframe where where https://github.com/jakubpawlowicz/clean-css/ inlined imports that we had this as a side effect? (AKA, when did they ship it?)

[wagenet]

@nathanhammond The chain is long and confusing.

It looks like ember-cli-preprocess-registry (the first package in the long chain to clean-css) was updated to ^2.0.0 from ^1.0.3 for ember-cli 2.3. Going back a step to ember-cli-preprocessor-registry shows us that 1.0.3 used broccoli-clean-css 0.2.0 which in turn uses clean-css ^2.2.1. ember-cli-preprocessor-registry 1.2.0 uses broccoli-clean-css ^1.1.0, which uses clean-css-promise which uses clean-css ^3.4.5.

So all that to say that prior to ember-cli 2.3 you could end up with clean-css ^2.2.1 or ^3.4.5 depending on what version of ember-cli-preprocessor-registry you were on. Once we hit 2.3 you were guaranteed to be on clean-css ^3.4.5.

Presumably the issues I'm seeing have something to do with that clean-css version change. This goes back long enough and is inconsistent enough that maybe we shouldn't do anything about this, though I really would like to make this less confusing to users, since I think that clean-css has the wrong defaults (especially in regards to importing // urls).

Another option that I realized is that we could do { processImportFrom: ['local'] } to still allow local imports but not allow remote. However, this might be considered a breaking change. Maybe this is something we could call out in the docs somewhere instead?

[nathanhammond]

@wagenet First, thank you for doing all of that research. That looks like it was a pain in the ass. In human time that means that we arbitrarily switched behavior on February 16, 2016 with no complaints (except yours), and could arbitrarily switch back with likely the same story. I consider this undefined behavior and am in favor of shipping a default configuration option of local.

Since it's easily configurable in consuming apps this seems like a solid win. I'm in favor. @rwjblue as caretaker of upgrade pain, you're the final arbiter on this.

[homu]

☔ The latest upstream changes (presumably #6629) made this pull request unmergeable. Please resolve the merge conflicts.

[rwjblue]

Lets do it!

[nathanhammond]

@homu r+

[homu]

📌 Commit bd3cc13 has been approved by nathanhammond

[homu]

⌛ Testing commit bd3cc13 with merge 4ea756a...

[homu]

💔 Test failed - status

[homu]

☀️ Test successful - status