Skip to content
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

Merged
merged 3 commits into from
Jul 19, 2013

Conversation

abarre
Copy link
Contributor

@abarre abarre commented Jul 19, 2013

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.

@GoalSmashers
Copy link
Contributor

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?

@abarre
Copy link
Contributor Author

abarre commented Jul 19, 2013

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.

@abarre
Copy link
Contributor Author

abarre commented Jul 19, 2013

Actually, after reading again the #85, it seems to be a different issue than the one we have.
In my case, we would need to fetch both the relative and absolute url from the origin server. #85 seems to consider only absolute url.

@GoalSmashers
Copy link
Contributor

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 -s / --skip-imports as that's an optional functionality. Would you dare to add them or should we pick it up from you?

@GoalSmashers
Copy link
Contributor

#85 will surely be a complicated one. Thanks for pointing that out.

"@import url(/fake.css);",
"@import url(/fake.css);"
]
}, {processImport : false})
Copy link
Contributor

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.

Copy link
Contributor Author

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 ?

Copy link
Contributor

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.

@GoalSmashers
Copy link
Contributor

@abarre looks great! Merging!

GoalSmashers pushed a commit that referenced this pull request Jul 19, 2013
Adds processImport / --skip-import option for avoiding @import statement processing.
@GoalSmashers GoalSmashers merged commit 18a1995 into clean-css:master Jul 19, 2013
@GoalSmashers
Copy link
Contributor

@abarre 1.0.12 is out in npm repository - thanks a million!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants