-
Notifications
You must be signed in to change notification settings - Fork 322
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
Add option processImport set to true by default. #121
Conversation
Thanks @abarre for the PR! We were thinking about handling non-local resources by fetching them and then optimizing (there's an issue opened for this: #85). Can you think of any other case when someone does not want to process imports? Not really sure what's the best way to handle that:
Thoughts? |
Thanks for consider the PR. In my opinion, you should consider to have them both : add an option and implement a basic version to fetch the non-local resource. In critical application, somebody could disable it because of performance concerns due to cost of fetching the file on the filesystem or over the network. If you want to implement an http request, be sure to let the possibility to override the request ( because of proxy, authentification, host header...). For instance, in our service, we override the Host header of the requests sent to origin server. |
Very good points @abarre. I totally agree with you regarding network considerations. We should also have a command line option to turn import processing off, something like |
#85 will surely be a complicated one. Thanks for pointing that out. |
"@import url(/fake.css);", | ||
"@import url(/fake.css);" | ||
] | ||
}, {processImport : false}) |
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.
Please add whitespace as we do elsewhere.
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 don't get the point. Can you explain or do the modification ?
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.
Sorry, just like this: { processImport: false }
to match formatting with other places.
@abarre looks great! Merging! |
Adds processImport / --skip-import option for avoiding @import statement processing.
@abarre 1.0.12 is out in npm repository - thanks a million! |
This pull-request add an option to select if we process the import.
For the integration of clean-css in our project, I need to disable the processing of the import statement.
The bad behavior we saw :
We use clean-css to minify a file that is downloaded from another server. So, clean-css doesn't find the css imported with relative path in the server that run the minifier and it just remove the line.
If you want to add this option to the binary, I can patch the pull-request. I don't think that it's necessary since the files will be likely local in this case.