test for relative urls in import (node less does not adjust relative paths)

[neonstalwart]

when using lessc on the command line, urls are not adjusted from imported style sheets. this adds a test case to show the failure. looking at previous issues it seems that this keeps popping its head up from time to time but there doesn't seem to be a test for it.

[neonstalwart]

this test is to show a very annoying long running bug in less. @cloudhead, do you mind adding the test so that patches can be made that will pass the test or at least let me know if the expected result is wrong so i can work on a new test case?

[marceloverdijk]

Hi, as you asked my view on this, I never use relative images.
I always use something like url('/images/myimage.jpg')

[neonstalwart]

@marceloverdijk i figured since it looks like you're building a tool for more than just yourself to use you might have an opinion about more than just what you always do.

how would you expect relative references to be resolved?

even if you use a simple css minifier, it will adjust the urls in imported style sheets. i would hope that less would do the same.

[marceloverdijk]

@neonstalwart you are right my tool should be usable for a broader audience than myself.
But at the moment I have no direct opinion how this should work. I will look into it.

[marklagendijk]

It seems there are a lot of issues with the functionality which rewrites the paths from @imported files from another directory. I think it is quite important that this gets fixed.

I will describe here how it should work. When I have more time (and no-one else has done it yet) I plan to implement / fix this and create a pull-request of it.

The main idea is that paths in a LESS file should be relative to that file. This means that if a LESS file @imports a file from another directory all the relative paths in the @imported file should be rewritten.

Example. We have the following file structure:

main.less
    imports
        subdirectory
            import.less

main.less:

@import "imports/subdirectory/import";

import.less:

.example {
    background-image: url('../images/bg.gif');
}

The output should be:

.example {
    background-image: url('imports/subdirectory/../images/bg.gif');
}

Which can be further optimized to:

.example {
    background-image: url('imports/images/bg.gif');
}

The code in less.js should do the following (in pseudo code):
Note: I don't know how much of this is already implemented.

path = resolveVariablesInPath(path);
if(currentFile.directory != mainFile.directory &&  isRelativePath(path){
    path = getPathToDirectory(mainFile.directory, currentFile.directory) + path;
}
if(this.outputDir != null){
    path = getPathToDirectory(this.outputDir, currentFile.directory) + path;
}
path = minifyPath(path);

/*
    Minifies file or folder paths to their shortest form
    Example:
        "./imports/subfolder/../../../images/test.png"
    Becomes:
        "../images/test.png"
*/  
function minifyPath(path){
    //remove occurences of './'
    var currentDirectoryPattern = /[^\.]\.[\/\\]/;
    while(path.match(currentDirectoryPattern) != null){
        path = path.replace(currentDirectoryPattern, path.match(currentDirectoryPattern)[0].substr(0,1));
    }

    //remove './' from the beginning of the path (the only case of './' not caught by our currentDirectoryPattern)
    if(path.indexOf("./") == 0 || path.indexOf(".\\") == 0){
        path = path.substr(2);
    }

    //remove 'someFolder/..' occurences
    var previousDirectoryPattern = /[^\/\\\.]+[\/\\]\.\.[\/\\]/;
    while(path.match(previousDirectoryPattern) != null){
        path = path.replace(previousDirectoryPattern, "");
    }

    return path;
}

Edit: adjusted the pseudo-code to work with outputDir.

[neonstalwart]

i believe it's even more involved because after normalizing the path to be relative to the file doing the import then you have to adjust the paths to be relative to the output file. when i looked into how to fix this, i found that the output path is not available anywhere and so this is why i stopped with a pull request for just a failing test.

relative paths have been an issue for as long as i can remember with less and so i would think that any help to fix them should be welcomed. the strange part is that since relative paths are so broken in less i guess barely anybody uses relative paths or else i would expect there would be more of an outcry.

[marklagendijk]

It quite suprised me that these issues have been around for this long without getting fixed. I suppose everyone must indeed be using absolute paths, which are fine for most use cases ( especially when you define your base path as a variable).

You are right that the paths ideally should be relative to the output file. Since the output file isn't known to less.js, I thought it would be best to just assume that:

1. either the output folder is the same as the input folder
2. althought the output folder isn't the same, the relative paths will still work when we act like it is (covers most use cases).

Now this isn't the ideal solution, of course. But I think it is at least a lot better than how it currently works (it doesn't).

Another solution would be to add something like a 'outputDir' parameter. This parameter would then be used when processing relative paths, and when it wouldn't be provided the directory of the main LESS file would be used.

Edit: I updated the pseudo-code in my previous comment with the usages of 'outputDir'.

[matthew-dean]

Crunch (the LESS editor) completely disables rewriting the URL within LESS because the default behavior confused me and I didn't see how it was giving any sort of useful output for a server-side compilation. When I'm writing LESS code, I'm already writing it based on where the image paths WILL BE in regards to the output CSS, not on where the images are now related to my LESS file. However, for client-side compilation, this kind of rewriting may be necessary in the conversion process, not sure.

In any case, I didn't like how LESS was rewriting URLs within background declarations so it made sense to remove that call.

It sounds like all of us have different expectations of what the URL should be relative to (the root LESS file, the imported LESS file, or the output CSS file) and how that URL, if rewritten, should appear, so it feels broken in different ways for each of us.

[neonstalwart]

@MatthewDL i'm not sure which of your categories i fall into but i expect urls to be written relative to the output css file - that's ultimately what matters when the browser loads the content. i author my paths to be relative to the LESS file since that seems to me like the most intuitive way to write relative urls.

in the context of client-side compilation there is still an "output css file" (ie a location that urls will be resolved against) so there shouldn't need to be any different logic in the client - it just happens that the input path and the output path are the same.

however because there can be different expectations i started (7 months ago) with a test that shows how the code currently fails. i realized i would be wasting my time to try and fix this until there is agreement on how it should work. we can tweak the test until we can agree that the test would enforce the expected behavior and then an implementation can be written to make the test pass.

i think we all would agree that relative urls just don't work right now though.

[gitawego]

I got the same problem as well, and it's not a difficult bug to be resolved, so hope it will be fixed (or pulled from other devs) asap :)

[loic]

First I'd like to voice that relative paths are extremely important, especially when you build an application that can be deployed at various depths (i.e. example.com/ and example.com/subfolder/) without configuration.

Let's look at the following example:

static/
    less/
        main.less
        imports/
            logo.less
            font.less
    css/
        background.css
        main.css
    assets/
        logo.png
        xecret.ttf

main.less:

@import "../css/background.css"

@import "imports/logo.less"
@import "imports/font.less"

main.css is the output of lessc for main.less.

This example and test cases for both client-side and server-side usages are available at https://github.com/loic/less.js/commit/745d0a2c7bf3e8b076b17f6f2d3c0d6029ec7199

Problem:
Currently there are two major inconsistencies between client-side and server-side usages:

1. Imports of .css files. (i.e. background.css in my example)
2. CSS declarations with URLs. (i.e. background-image, @font-face, etc.)

Objective:
There is more than one way to deal with these, and how we do it is not even the most important, what matters is:

1. Client-side and server-side usages should provide a consistent output.
2. Whatever convention we adopt should be documented once and for all.

Possible solution 1:
Right now lessc leaves all the relative path declarations as-is, we can settle on that and write all our imports and URLs relative to the output file.

For client-side usage we could agree that the equivalent of the .css file is the .less file linked in the HTML and rewrite all the imports and URLs relative to it.

One advantage of this solution is that we just need to fix client-side usage, as this already works with server-side usage.

Possible solution 2:
We could write all our imports and URLs relative to the file that defines them.

In that case both client-side and server-side usages would require rewriting all the URLs relative to the final output file.

This solution has an extra advantage over solution 1 for the edge case where one would like to import a .css or .less file multiple times for different output .css files that could live in arbitrary locations. It's a bit twisted, but not inconceivable.

One might argue that it's also easier to write the path relative to the file you are currently editing rather than from the file where it might be imported later on.

I would be happy with either of the above solutions, there might even be other ways to do it, as long as client-side and server-side usages are finally interchangeable.

[loic]

I implemented the first solution as it's the low hanging fruit.

Basically it requires passing around the path of the top importing stylesheets. Since a similar hack is already in place to pass around sheet.contents maybe it would be better to refactor and make the top stylesheet and various data about it available throughout the parsing process.

This fix makes the assumption that all relative paths in the generated CSS in browser mode have for reference an imaginary .css file that lives in the same folder as the top importing .less file.

When the real .css file generated by lessc and the top importing .less file are in the same folder, all relative paths should be compatible.

When they are NOT in the same folder, relative paths must be written in a way where they will resolve to the same directory. This implies that the top importing .less and generated .css files are at the same depth on the file system.

Further thoughts:

I'm rather convinced that solution 2 is better. It basically involves one more step that should be performed for both lessc and browser mode. If we all agree that's what we want, I'll have a go at implementing it.

This fix helps with relative @imports of .css files but I believe strongly these should be imported inline as suggested in #894. It's good to note that if we go ahead with a fix for #894 we'll have to revisit url rewriting for relative paths.

I've had a look at how to integrate the tests for this issue with the rest of the automated tests but since it requires rendering from a web browser I'm not sure how to proceed. Any hints?

[lukeapage]

@loic thanks, will take a look, though probably not before the weekend.

Feel free to do a pull request to make this dicussion clearer. if possible don't include a build of less.js 1.3.0 in your code.

[loic]

@agatronic no worries, I will do that this evening.

For the build I figured it would be needed for test_331 to work out of the box since I didn't manage to integrate with the test framework but then I realized it wasn't ideal so I made a7f4063 without it.

I'll work out the kinks in the pull request and simply document how to run and update test_331 manually.

[SalimBensiali]

Hi all,
I implemented Solution 2 that @loic described. Feel free to provide feedback. See #976.

[lukeapage]

I think discussion has moved on from this to the pull requests where solutions are implemented.

Plus we plan initially to bring the two behaviours inline by going with non adjustment of paths. We'll treat the pulls to do relative re-writing as a feature request.

[stej]

In what version can we expect the pull requests will be available in public release?

[lukeapage]

@stej a new version has been released (1.3.3) and this has 2 options - rootpath and relativeUrls
http://lesscss.org/#usage

Those options address all the known requirements.