v3.3.1 breaks @import pathing
Closed this issue · 21 comments
It appears as though the latest version of gulp-clean-css has a different behaviour with how it handles @import paths using the inline: ['none'] option.
We unintentionally installed v3.3.1 by using the package.json devDependency declaration: "gulp-clean-css": "^3.2.0".
We fixed the issue locally by changing our package.json devDependency declaration to exact: "gulp-clean-css": "3.2.0".
See screenshots below
[v3.2.0] expected result:
[v3.3.1] unexpected result:
I think I may have a fix for this. Before I go further, can you provide me a snippet of that same css before it's minified? I'd like to see the paths beforehand
The screenshot above for v3.2.0 is how the URLs are with or without minification. Setting the rebaseTo option to false, '', or my destination folder doesn't seem to fix it either.
Minification turned off:
Interesting, I shouldn't be resolving any paths by default. Could you try to add a leading slash to your import? e.g.
@import url('/app.min.[...]')
For some reason my test broke when I removed the leading slash with the new changes in 3.3.1. Also, rebase has not been surfaced yet as discussed in #42 - it should be false by default unless rebaseTo is specified. I will be surfacing rebase as configurable very soon should the need for it arise, but I don't suspect that'd be the cause of the issue here. Hm, leading slash...
With leading slash:
The issue here is that it is still an undesired result. I need it to respect the adjacent directory @import as it was doing previously.
@import url(/app.min.css);
Will try and import from the hostname root -- which doesn't work if the assets are located in an assets folder. Ex path: "http://my-domain.com/assets/css/app.min.css" don't click
Although, I could use absolute pathing as the example I just mentioned showed -- there should be no need to introduce an absolute URL here.
Assets folder:
Have you tried rebasing at all, perhaps rebaseTo: 'assets/css'?
Yes. As mentioned earlier, setting the rebaseTo option to false, '', or my destination folder doesn't seem to fix it either.
rebaseTo: 'assets/css'
What I think might be the culprit is if gulp-clean-css is somehow trying to inherit the path from gulp.src(). The src is directed at my sass entry point, which is referred to in a relative manor. See screenshot below. I just need it to not rebase at all and leave the URLs as they are.
Thanks again for your help on this. I appreciate it.
Interesting. Okay, so, everything was working for you find beforehand? I made some fundamental changes to the way the file is passes to clean-css for the sake of rebasing as seen here 92ddf63
I wonder if, I can surface another option specific to the plugin which can flag to alternatively pass in the direct inline styles such as I was doing before? That way, we can satisfy those who need to rebase and present the option for folks to not concern themselves with it whatsoever. Perhaps something like inlineStream: true (hopefully not to be confused with the inline option from clean-css). Thoughts?
Yes, it was working before. I was doing some digging and I'm not sure clean-css is respecting the rebase: false option.
Looking at: gulp-clean-css\node_modules\clean-css\lib\reader\rebase.js
It appears that rebase will run if rebaseAll is set to true or false. I'm not familiar enough with the codebase to know where exactly this is originating from -- or if rebaseAtRules() is supposed to run if rebase: false; is passed.
If I just return the tokens if rebaseAll is false in the rebase function, it works.
Without my addition:
Can you verify if this is an issue with how the paths or options being set by gulp-clean-css are being passed in; or rather an issue with clean-css proper not respecting the rebase: false option.
So I looked into this a bit and the only difference your flow should be experiencing is the now passing of the file style contentsas a keyed object that clean-css expects for rebasing e.g.
toPass = {
[file.path]: {
styles: contents
}
};
Before these changes, I was passing contents in directly. I am assuming there is some conflict with your processing of SASS beforehand, and then the keyed filename of that object resolving something literally that was not occurring when just the raw content was passed in.
So, I intend to include a plugin-level option to allow to fallback to the prior passing of styles - something like isolatePipeline: true would pass styles, preprocessed or not, inline. Including this option should emulate the same functionality of 3.2.0. I'll get a change pushed up later on this evening and share you a locally packed up tarball to install and try out. Would that be okay?
That sounds good.
Usually the gulp plugins I have used in the past cater to the contents being piped in without regard to the where the content actually comes from. That is part of why they are so flexible.
return gulp.src('file')
.pipe(pluginA(options))
.pipe(pluginB(options));
if pluginA and pluginB were in reverse order, it still usually works but is a different order of operations on transforming the content. Neither plugin cares about the specifics of gulp.src() more than the contents that gulp.src() pipes in.
Providing an option to isolatePipeline will definitely solve this issue, but it almost feels like it is going against the grain. Perhaps it should be reversed and gulp-clean-css should be isolatePipeline by default with the option to explicitly state the src path for use with rebasing. Technically, we could imply that the user wants to isolatePipeline when rebase: false is being passed into the clean-css options. This would prevent the need to add another option altogether.
Either way works for my instance, but I would be curious to see how you and other users feel about it. Obviously, I recognize that setting default behavior can be dangerous for existing users.
To summarize, the 3 options suggested are:
- Imply the the user wants an isolated pipeline when they specify the already existing clean-css rebase property:
gulp-clean-css({ rebase: false }) - Isolate the pipeline by default and only imply the rebase path by setting a new property such as isolatePipeline.
- Require the user to explicitly state they want an isolated pipeline when using gulp-clean-css by setting a new property such as isolatePipeline.
My preference would be option 1 or 2 as I think they are the least invasive and most implicit of the clean-css options.
Thanks for your support on this and let me know when you have something I can test locally.
I like your suggestion! In-versing my proposal makes sense with how you put it. Hopefully I can free up soon to get this done, at the latest, I can get the changes in by the end of the weekend - so - hopefully as a workaround until then you can keep moving forward with 3.2.0. Thank you for the feedback!
Sure thing! I edited my response early to provide a little more clarity.
Take your time, but I'm happy to assist whenever you have something for me test.
Hey again. So after re-reading this thread I made a super quick and dirty change to simple respect the rebase: false. As a result my tests appear to mimic the output as they did in 3.2.0. I packed up a tarball at https://github.com/scniro/gulp-clean-css-43 for you to try out in isolation before I publish anything to see if I can solve your issue. When you get the time, could you pull it down to test locally? Please share your findings. Thanks!
@scniro, I'm testing this locally, but it doesn't look like it is working. Can you verify what the change made was in the code? I'm not sure if it carried over in the tarball.
@stephencorwin hey, sorry if it's a wasted effort on your issue 😦 The change should respect rebase: false if passed in. @sassy-ankit thank you very much for exploring this as well. I can slate some time this weekend to squash the issue! Also, if feeling adventurous, feel free to submit a PR. Not at all necessary, just throwing it out there. Get back to you guys later on this weekend
@sassy-ankit
Forcing the use of another dependency (plumber) when it wasn't required before is not an acceptable solution in my opinion. It wasn't required before version 3.2.0. Any project looking to upgrade and use the latest version of gulp-clean-css should not need to rewrite their pipeline to use plumber.
To best support the direction that clean-css proper is going we should aim towards supporting the option rebase: false implicitly.
@scniro
I'll take another shot at getting it to work locally over the weekend. Thanks again for your support.
@stephencorwin okay I tried again...
This time the rebase option will still be configurable, but, I have reverted to the old way of passing in the direct styles opposed to the keyed file name object. The object will only be passed should one specify rebaseTo. Install and let me know?
@scniro, I believe this is working.
no rebase option set:
//rebaseTo: '/test/'
rebaseTo option set:
rebaseTo: '/test/'
I haven't used rebaseTo intentionally, before -- so I'm not positive this output is correct with rebaseTo set. Please confirm that rebaseTo works as intended when set. It works for my needs without it being set.
If this change doesn't break any other tests, then I think we should be good to go.
@stephencorwin yep you should be good to go and I can publish up these changes this evening. Shouldn't affect anyone as passing the styles directly has always been the method I leveraged for 3.20 and before , but now for those who use rebaseTo - they internally opt for passing the keyed object. I'll comment here later on once I bump to npm and let you close it out once all looks good. I appreciate your efforts and the collaboration on this!
@stephencorwin @sassy-ankit 3.4.0 is now live and should be available to pull down via npm. Please let me know if you are squared away on this! Thanks. Also I ditched the isolatePipeline proposal as I think the special case for rebasing will suffice.
Thanks @scniro! I think we can close this issue out.