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

@import inlining #47

Closed
santiagoaguiar opened this issue Feb 28, 2013 · 8 comments
Closed

@import inlining #47

santiagoaguiar opened this issue Feb 28, 2013 · 8 comments

Comments

@santiagoaguiar
Copy link

Hi! I'm having some issues trying to flatten a CSS file that includes @imports (so that the imports get inlined with the CSS) and at the same time using require-css optimizer to inject the file in the JS.

The optimized JS file still includes the @imports directives, but I think it should either inline them, or even better, require them so that there's a single declaration of the imported CSS in the JS output.

Is this possible? I read something about @import handling on css.js, but that doesn't seems to be used when building, right?

I'm guessing that maybe this might work out of the box when r.js runs over an entire appDir, but I need to run it over the individual files, since the project folders contains other stuff which I don't want r.js to touch.

@guybedford
Copy link
Owner

Hi Santiago,

Yes require-css currently relies on the optimizer to inline the imports as
part of a whole project build. To work with single file builds I will add
this as a feature request to the require-css project.

Guy

On Thursday, February 28, 2013, Santiago Aguiar wrote:

Hi! I'm having some issues trying to flatten a CSS file that includes
@imports (so that the imports get inlined with the CSS) and at the same
time using require-css optimizer to inject the file in the JS.

The optimized JS file still includes the @imports directives, but I think
it should either inline them, or even better, require them so that there's
a single declaration of the imported CSS in the JS output.

Is this possible? I read something about @importhttps://github.com/importhandling on css.js, but that doesn't seems to be used when building, right?

I'm guessing that maybe this might work out of the box when r.js runs over
an entire appDir, but I need to run it over the individual files, since the
project folders contains other stuff which I don't want r.js to touch.


Reply to this email directly or view it on GitHubhttps://github.com//issues/47
.

@guybedford
Copy link
Owner

I've included this in the latest version on the dev branch of the module. I'm still finalizing this 0.1 version, and will post a proper release out after more testing, but it should be fine to use in the mean time.

@santiagoaguiar
Copy link
Author

Great! I'll test it out in a couple of hours and let you know, thanks for considering this!

@guybedford
Copy link
Owner

This is now in the master branch - please report if you have any issues.

@santiagoaguiar
Copy link
Author

Sorry for the late reply!

The change seems to work ok! I found a couple of issues:

css-builder.js:

147c147
<       cssBase = config.cssBase || config.appDir || baseUrl;
---
>       cssBase = config.cssBase || config.appDir || config.baseUrl || baseUrl;

I assumed that was the intention. Otherwise cssBase was being set to null. I suppose I could set cssBase property instead on the config, but I guess it should default to config.baseUrl otherwise.

css.js:

157c157
<     if (bufferResources[resourceId]) {
---
>     if (indexOf(bufferResources, resourceId) != -1) {

Otherwise the resource was still requested for download.

After those two changes, things seems to be working wonderfully good :).
Thanks for taking the time to add this.

@guybedford
Copy link
Owner

Well caught - really appreciate your spotting these. This may well also resolve #56.

In terms of moving forward with this cssBase property, I would value your opinion on requirejs/r.js#412 if you care to comment.

@santiagoaguiar
Copy link
Author

I thought that the cssBase was being use to resolve require dependencies, not for CSS import statements. For require dependencies, I think would be a useful feature. In my projects I end up having to write things like:

css!../../css/moduleA/myModule.css

Because, AFAIK, the css is resolved based on the baseUrl, which in my case points to js/. I would prefer to be able to specify a baseUrl for the css plugin, so that I could write css!moduleA/myModule.css and set its baseUrl to css/. Maybe there's already a way to do that.

About jrburke/r.js#412, I don't have a strong opinion, I agree that supporting / on import urls might be useful, though I always use relative URLs on my web projects to support deployment under different paths.

I think that if you use '/' is because you expect a specific configuration on your environment, so it might be the case that the stuff you have on '/' is actually shared by multiple modules. But I'll agree that having '/' to mean "shared don't optimize" is a bit obscure ;).

@guybedford
Copy link
Owner

css! dependencies are resolved using exactly the same logic as normal js requires in RequireJS. That is it works with map and paths config etc. And all relative to the base url.

The reason I added cssBase is because in normal requires, css!some-file means relative to baseUrl, but in CSS imports there is no way to reference the baseUrl, only the site root. Hence the need to specify the site root relative to the baseURL for the optimizer to know where these inline from.

But yeah I think relative imports is the best way forward - any module will typically know its "reverse distance" from the baseUrl, but at least the cssBase is supported anyway just in case someone needs it.

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

No branches or pull requests

2 participants