Support protocol @imports #85

XhmikosR · 2013-03-22T09:53:50Z

Example:

@import url(http://weloveiconfonts.com/api/?family=zocial);

GoalSmashers · 2013-07-19T12:12:50Z

As @abarre commented in #85 - we should consider:

handling relative URLs
overriding request options because of proxy, authentification, host header...

abarre · 2013-07-19T12:42:04Z

Regarding this issue, I'm not sure that it's a real good idea.
You might end up with an outdated css if the external css is updated afterwards. It's a bad side effect.

GoalSmashers · 2013-07-19T12:53:40Z

Let's see if anyone asks for it. Maybe there's no real "business" value behind it.

XhmikosR · 2013-07-19T15:49:53Z

I'm not sure I follow... Wouldn't it be simpler to just keep the import at the beginning instead of going all through the trouble to download the css etc?

abarre · 2013-07-19T16:14:26Z

@XhmikosR, it's also my opinion.

GoalSmashers · 2013-07-19T22:01:49Z

Sure, let's see if there's any interest in it.

terinjokes · 2013-10-31T03:41:08Z

I officially register interest in such feature.

GoalSmashers · 2013-10-31T07:57:17Z

@terinjokes can you share your use case if it's any different from the example on top?

terinjokes · 2013-10-31T08:42:40Z

Have a tool that creates static versions of webpages by inlining resources. Right now I'm combing though CSS with parserlib, reconstructing the stylesheet and inlining the resources as I come to them. Because this doesn't minify the CSS, I then run the reconstructed stylesheet through clean-css.

Seems a bit wasteful to parse the stylesheet twice.

If you don't want to handle setting up the fetches, I'm happy to take a callback.

GoalSmashers · 2013-10-31T10:18:05Z

@terinjokes Thanks for your use case!

Since @abarre suggested we should expose an ability to override request properties (in case of proxies, authentication, etc) it makes me wonder if it's doable via CLI or should we make it the lib option only.

A timeout option may be enough for CLI.

GoalSmashers · 2013-11-30T15:19:20Z

Since fetching external data will be asynchronous and clean-css is synchronous, here's an idea how to handle it.

We add an optional callback to minify method:

new CleanCSS().minify(source, function(clenCSSInstance, minified) {
...
});

Output debug info to stderr every time external URL is processed.
Makes sure URL resolving is recursive.
If callback not given add warnings on every external URL found.
Still support synchronous mode the same way as we do now.

CLI will have a timeout option only. Overriding any http.get / https.get option will be possible when using the library directly.

terinjokes · 2013-11-30T22:59:35Z

Sounds like a good start from this end, I'm already using the library.

GoalSmashers · 2013-12-01T09:51:02Z

Great! I should have some spare time to push it forward in the coming days.

* Rewrote inliner to process data asynchronously. * Supports 2xx responses, redirects, errors, and timeouts. * Supports cyclical references. * Supports protocol-less requests too (defaults to HTTP). * Supports overriding request options - see http://nodejs.org/api/http.html#http_http_request_options_callback * Supports timeout in binary via --timeout / -t switches.

GoalSmashers · 2013-12-10T19:27:26Z

@terinjokes @abarre if you would like to give it a try then a preliminary but fully functional version is on f-imports-85 branch.

No changes are necessary when running via CLI.
When minifying via a library make sure to use a callback as fetching all data from remote hosts is asynchronous, i.e.:

new CleanCSS().minify(source, function(errors, minified) {
  ...
});

One can also override requests' options via:

var options = {
  inliner: {
    request: { ... }
  }
};
new CleanCSS(options).minify(source, function(errors, minified) {
  ...
});

terinjokes · 2013-12-10T20:02:36Z

Overall, it looks good, but some feedback before we go any farther.

If a callback is provided to minify it automatically resolves external resources as this.options.processImport defaults to true. That's a breaking API change because of this next point.

When this.options.processImport is true, the callback could be called on a future tick (via inlineRemoteResource). Previously, and when this option is false, the callback is called on the same tick. This releases Zalgo. 😦

GoalSmashers · 2013-12-10T20:23:10Z

@terinjokes Thanks for the feedback - the thing is callback handling is not finished yet as minify() will run the very same way as it runs now so API will be backwards compatible.

Thanks for pointing current/next tick thing. Will make sure it's always deferred.

terinjokes · 2013-12-10T20:24:34Z

You can't be API backwards compatible and change when the callback gets called. I think that's as much as part of the API as the method signatures.

GoalSmashers · 2013-12-10T20:26:10Z

But there is no callback in the current version of clean-css. It will be added in 2.1.

terinjokes · 2013-12-10T20:27:12Z

D'oh! It was already there in the commit added above, so I assumed it was already on master.

GoalSmashers · 2013-12-10T20:30:33Z

No worries - this branch adds callbacks too!

@imports

* Rewrote inliner to process data asynchronously. * Supports 2xx responses, redirects, errors, and timeouts. * Supports cyclical references. * Supports protocol-less requests (defaults to HTTP). * Supports overriding request options - see http://nodejs.org/api/http.html#http_http_request_options_callback * Supports timeout in ./bin/cleancss via --timeout / -t switches. * Supports inlining local resources only without a callback. * Supports rebasing URLs in remote @imports. * Always triggers callback asynchronously.

@imports

* Rewrote inliner to process data asynchronously. * Supports 2xx responses, redirects, errors, and timeouts. * Supports cyclical references. * Supports protocol-less requests (defaults to HTTP). * Supports overriding request options - see http://nodejs.org/api/http.html#http_http_request_options_callback * Supports timeout in ./bin/cleancss via --timeout / -t switches. * Supports inlining local resources only without a callback. * Supports rebasing URLs in remote @imports. * Always triggers a callback asynchronously.

GoalSmashers · 2013-12-11T16:01:01Z

@terinjokes That should be it. Callback will always be deferred via process.nextTick even when resolving local assets. It is now also possible to resolve local imports when not using a callback and remote URLs (images, fonts, etc) are also rebased.

Not sure if we should go for Output debug info to stderr every time external URL is processed. as in my comment above since I see no real value in it.

What do you think?

terinjokes · 2013-12-11T19:26:04Z

I don't think that's really an error, maybe if you had sort of debug mode on.

I'll give it a run this afternoon and let you know.

@imports

* Rewrote inliner to process data asynchronously. * Supports 2xx responses, redirects, errors, and timeouts. * Supports cyclical references. * Supports protocol-less requests (defaults to HTTP). * Supports overriding request options - see http://nodejs.org/api/http.html#http_http_request_options_callback * Supports timeout in ./bin/cleancss via --timeout / -t switches. * Supports inlining local resources only without a callback. * Supports rebasing URLs in remote @imports. * Always triggers a callback asynchronously.

GoalSmashers · 2013-12-11T21:13:40Z

@terinjokes Cool, thanks! It indeed makes sense to show that info under debug mode - thanks for the tip.

@imports

* Rewrote inliner to process data asynchronously. * Supports 2xx responses, redirects, errors, and timeouts. * Supports cyclical references. * Supports protocol-less requests (defaults to HTTP). * Supports overriding request options - see http://nodejs.org/api/http.html#http_http_request_options_callback * Supports timeout in ./bin/cleancss via --timeout / -t switches. * Supports inlining local resources only without a callback. * Supports rebasing URLs in remote @imports. * Always triggers a callback asynchronously.

@imports

* Rewrote inliner to process data asynchronously. * Supports 2xx responses, redirects, errors, and timeouts. * Supports cyclical references. * Supports protocol-less requests (defaults to HTTP). * Supports overriding request options - see http://nodejs.org/api/http.html#http_http_request_options_callback * Supports timeout in ./bin/cleancss via --timeout / -t switches. * Supports inlining local resources only without a callback. * Supports rebasing URLs in remote @imports. * Always triggers a callback asynchronously.

GoalSmashers · 2013-12-21T10:47:11Z

@terinjokes Did you have a chance to give it a try? Would like to merge it into master soon.

terinjokes · 2013-12-21T19:25:44Z

@GoalSmashers No, been working on other things. going to try it now.

terinjokes · 2013-12-21T19:41:30Z

I get an error on inliner.js:43 where data is an object that doesn't have the function "indexOf".

GoalSmashers · 2013-12-21T23:53:51Z

Thanks for checking! Could you give it a try with --debug flag to find out it fails on a local or remote import (the latter one I presume).
Additionally if it inlines publicly available assets could you share your CSS at dev (at) goalsmashers.com?

On Dec 21, 2013, at 8:41 PM, Terin Stock notifications@github.com wrote:

I get an error on inliner.js:43 where data is an object that doesn't have the function "indexOf".

—
Reply to this email directly or view it on GitHub.

@imports

* Rewrote inliner to process data asynchronously. * Supports 2xx responses, redirects, errors, and timeouts. * Supports cyclical references. * Supports protocol-less requests (defaults to HTTP). * Supports overriding request options - see http://nodejs.org/api/http.html#http_http_request_options_callback * Supports timeout in ./bin/cleancss via --timeout / -t switches. * Supports inlining local resources only without a callback. * Supports rebasing URLs in remote @imports. * Always triggers a callback asynchronously.

GoalSmashers · 2014-01-03T11:46:06Z

@terinjokes The issue you've reported regarding indexOf was very likely fixed in 0dbb54a - would be great if you could give it a try now so we can proceed with this feature.

terinjokes · 2014-01-03T19:24:50Z

Works now, however are there any chance to dictate how to inline other, non-css, resources?

Sorry, I didn't see the other comment.

GoalSmashers · 2014-01-03T19:32:57Z

Don't worry, I'm just happy it works well now!

To inline other resources you can pipe output of clean-css to https://github.com/GoalSmashers/enhance-css - is it an option for you?

@imports

* Rewrote inliner to process data asynchronously. * Supports 2xx responses, redirects, errors, and timeouts. * Supports cyclical references. * Supports protocol-less requests (defaults to HTTP). * Supports overriding request options - see http://nodejs.org/api/http.html#http_http_request_options_callback * Supports timeout in ./bin/cleancss via --timeout / -t switches. * Supports inlining local resources only without a callback. * Supports rebasing URLs in remote @imports. * Always triggers a callback asynchronously.

@imports

* Rewrote inliner to process data asynchronously. * Supports 2xx responses, redirects, errors, and timeouts. * Supports cyclical references. * Supports protocol-less requests (defaults to HTTP). * Supports overriding request options - see http://nodejs.org/api/http.html#http_http_request_options_callback * Supports timeout in ./bin/cleancss via --timeout / -t switches. * Supports inlining local resources only without a callback. * Supports rebasing URLs in remote @imports. * Always triggers a callback asynchronously.

@imports

* Rewrote inliner to process data asynchronously. * Supports 2xx responses, redirects, errors, and timeouts. * Supports cyclical references. * Supports protocol-less requests (defaults to HTTP). * Supports overriding request options - see http://nodejs.org/api/http.html#http_http_request_options_callback * Supports timeout in ./bin/cleancss via --timeout / -t switches. * Supports inlining local resources only without a callback. * Supports rebasing URLs in remote @imports. * Always triggers a callback asynchronously.

GoalSmashers · 2014-01-06T18:40:26Z

Merged to master to be included in 2.1. Thanks for all the help!

Slava · 2014-01-11T02:16:13Z

Hi, I think this is not resolved:

cleanCss = require('clean-css');
inst = new cleanCss;
inst.minify('@import url("//fonts.googleapis.com/css?family=Oleo Script Swash Caps");\n.block2 { font-family: \'Oleo Script Swash Caps\'; }\n');

resolves into

@import url(//fonts.googleapis.com/css?family=Oleo) Script Swash Caps);.block2{font-family:'Oleo Script Swash Caps'}

Which is obviously wrong, look at the "Oleo) Script Swash Caps", why is the closing bracket found after "Oleo"?

GoalSmashers · 2014-01-11T14:24:42Z

@Slava thanks for bringing that up - have just opened an issue in your name. We will deal with it quickly!

ghost assigned GoalSmashers Mar 22, 2013

GoalSmashers mentioned this issue Jul 19, 2013

Add option processImport set to true by default. #121

Merged

GoalSmashers closed this as completed in 2b2b586 Jan 6, 2014

GoalSmashers mentioned this issue Jan 11, 2014

Bug in protocol imports #207

Closed

terinjokes mentioned this issue Mar 12, 2014

Can't force the embed of all resources? jakubpawlowicz/enhance-css#19

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support protocol @imports #85

Support protocol @imports #85

XhmikosR commented Mar 22, 2013

GoalSmashers commented Jul 19, 2013

abarre commented Jul 19, 2013

GoalSmashers commented Jul 19, 2013

XhmikosR commented Jul 19, 2013

abarre commented Jul 19, 2013

GoalSmashers commented Jul 19, 2013

terinjokes commented Oct 31, 2013

GoalSmashers commented Oct 31, 2013

terinjokes commented Oct 31, 2013

GoalSmashers commented Oct 31, 2013

GoalSmashers commented Nov 30, 2013

terinjokes commented Nov 30, 2013

GoalSmashers commented Dec 1, 2013

GoalSmashers commented Dec 10, 2013

terinjokes commented Dec 10, 2013

GoalSmashers commented Dec 10, 2013

terinjokes commented Dec 10, 2013

GoalSmashers commented Dec 10, 2013

terinjokes commented Dec 10, 2013

GoalSmashers commented Dec 10, 2013

GoalSmashers commented Dec 11, 2013

terinjokes commented Dec 11, 2013

GoalSmashers commented Dec 11, 2013

GoalSmashers commented Dec 21, 2013

terinjokes commented Dec 21, 2013

terinjokes commented Dec 21, 2013

GoalSmashers commented Dec 21, 2013

GoalSmashers commented Jan 3, 2014

terinjokes commented Jan 3, 2014

GoalSmashers commented Jan 3, 2014

GoalSmashers commented Jan 6, 2014

Slava commented Jan 11, 2014

GoalSmashers commented Jan 11, 2014

Support protocol @imports #85

Support protocol @imports #85

Comments

XhmikosR commented Mar 22, 2013

GoalSmashers commented Jul 19, 2013

abarre commented Jul 19, 2013

GoalSmashers commented Jul 19, 2013

XhmikosR commented Jul 19, 2013

abarre commented Jul 19, 2013

GoalSmashers commented Jul 19, 2013

terinjokes commented Oct 31, 2013

GoalSmashers commented Oct 31, 2013

terinjokes commented Oct 31, 2013

GoalSmashers commented Oct 31, 2013

GoalSmashers commented Nov 30, 2013

terinjokes commented Nov 30, 2013

GoalSmashers commented Dec 1, 2013

GoalSmashers commented Dec 10, 2013

terinjokes commented Dec 10, 2013

GoalSmashers commented Dec 10, 2013

terinjokes commented Dec 10, 2013

GoalSmashers commented Dec 10, 2013

terinjokes commented Dec 10, 2013

GoalSmashers commented Dec 10, 2013

GoalSmashers commented Dec 11, 2013

terinjokes commented Dec 11, 2013

GoalSmashers commented Dec 11, 2013

GoalSmashers commented Dec 21, 2013

terinjokes commented Dec 21, 2013

terinjokes commented Dec 21, 2013

GoalSmashers commented Dec 21, 2013

GoalSmashers commented Jan 3, 2014

terinjokes commented Jan 3, 2014

GoalSmashers commented Jan 3, 2014

GoalSmashers commented Jan 6, 2014

Slava commented Jan 11, 2014

GoalSmashers commented Jan 11, 2014