add moduleUrl option

[sokra]

This PR adds an option which allows to switch the mode for relativeUrls.

By default url(./file) is emitted as url(file) and url(module/path) is emitted as url(folder/module/path) when imported from another file in folder.

With moduleUrl = true url(./file) is emitted as url(./file) and url(module/path) is emitted as url(module/path) even when imported from another file.

Why? CSS Modules use CommonJs-like url(...)s.

[seven-phases-max]

By default ... url(module/path) is emitted as url(folder/module/path)

Could you elaborate? Because by default (i.e. without --relative-urls) it is actually url(module/path). So I'm not sure I understand how this option is different from simply not using -ru.

Aside of above it would be extremely useful to provide some example of a practical use-case this option is intended to cover.

[sokra]

This is the current behavior:

@import "folder/file.less";

/* folder/file.less */
.a { background: url(img.png); }

Result without relative-urls:

.a { background: url(img.png); }

Result with relative-urls:

.a { background: url(folder/img.png); }

That's fine for normal CSS.

---

But with CSS Modules urls should behave like require in CJS/AMD:

/* folder/file.less */
.a { background: url(./img.png); }
.b { background: url(module/img.png); }

should result in

.a { background: url(./folder/img.png); }
.b { background: url(module/img.png); }

But currently it results in: (with relative-urls)

.a { background: url(folder/img.png); }
.b { background: url(folder/module/img.png); }

With this PR (with relative-urls plus module-url):

.a { background: url(./folder/img.png); }
.b { background: url(module/img.png); }

---

The original problem was that url(./img.png) is emitted as url(img.png) by less.js.
webpack-contrib/css-loader#74

[seven-phases-max]

In other words, the purpose of this feature is to always interpreter path starting with ./ as path relative to current file, is it?

Then, hmm, I'd rather suggest a slightly different solution. Instead of an option hardcoded as a back-patch for the --relative-urls, it would make sense to have an option orthogonal to either --relative-urls state, because forcing --relative-urls (= translate path paths relatively to current file) first and then cancelling it with --moduleUrl (cancel path translation for generic paths but keep it for ./) looks quite inconsistent.
I.e. what about an option that just changes the interpretation of ./ paths themselves?
e.g.:

url(./a.png);    // -> always translate relatively to current file
url(./../a.png); // -> always translate relatively to current file
url(a.png);      // -> behave according to `-ru`
url(../a.png);   // -> behave according to `-ru`

e.g. for your use-case (and for many others) you'd use just that option w/o --relative-urls. This will also solve many problems with urls in a projects where you have to mix stuff written with different --relative-urls settings in mind (currently it's quite a problem that needs a solution too, and if it will have to be yet another options... ouch...).

---

Theoretically I would even suggest even more radical solution. Instead of using ./ as a demarcation for those things, personally I think I would prefer something yet more explicit like just another function: e.g. an rurl() or urlr() (or whatever()) function meaning "always relative to current file", so I could just set it explicitly and never rely-on/guess-of any external options (pretty much the same thing as was suggested in #2560).

---

P.S. #2560 reminds me that all those ./ or rurl() will still be problem if used in a mixin, but here it does not look as critical as for data-uris... And it's sort of out of scope of this feature-request anyway (I just mention this as a self-note).

[sokra]

I could change the relativeUrls option from true/false to true/false/"explicit". Would this make it more consitent?

"explicit" because you need to explicitly add ./ or ../ to make url(...)s relative.

[seven-phases-max]

I could change the relativeUrls option from true / false to true / false / "explicit" .

this make sense, but let's wait for other opinions...

[markdalgleish]

Any word on this? I'm eagerly awaiting the result of this PR 😊

[seven-phases-max]

@markdalgleish Well, not until there're more opinions and there's some consensus of how exactly it should be solved (since it's several ways with their pros and cons).

[markdalgleish]

I think including it in the relativeUrls option is somewhat confusing, but I understand the reasoning.

IMO the fact that the output URLs are specifically designed for CSS Modules—always starting with a . unless they're module URLs (e.g. from node_modules)—makes a strong case for referencing CSS Modules in the new option somehow. Maybe the options could be true / false / "modules"?

[matthew-dean]

what about an option that just changes the interpretation of ./ paths themselves?

I'm in agreement with this. It seems unlikely to break anything, since writing url(./img.png) is an unusual pattern in normal circumstances. So to make it folder-relative seems fine. I'd prefer that rather than patching with more options. Like @seven-phases-max, the intent of relative URLs as far as a switch is to express a specific type of behavior, which is dissimilar to this module behavior, since--same complaint--it cancels relative paths in some circumstances.

It seems fine to make it default because the workaround, if someone HAD been intentionally writing ./ at the beginning of their urls, is to delete two characters. So it's theoretically a breaking change, although I can't imagine it has much real-world usage. Let's just make ./ folder relative, regardless and drop module-url. My $0.02.

[markdalgleish]

So does this mean that if relativeUrls is false, that the following:

.a { background: url(./folder/img.png); }
.b { background: url(module/img.png); }

...would be emitted as-is, without the URLs being rewritten by Less?

[seven-phases-max]

@markdalgleish Yes (http://lesscss.org/usage/#command-line-usage-relative-urls).

[sokra]

emitted as-is is not enough.

url(./folder/img.png) must be emitted as url(./sub/folder/img.png) when imported from a file in sub folder. see example here: #2615 (comment)

[matthew-dean]

@sokra Yes, @seven-phases-max is talking about current behavior. The proposed behavior is as you say, that a ./ would prepend the sub folder of the file making the URL request. But... maybe it does need to be more explicit because it might get confusing.

What about doing this:

1. Create an option called "rewrite URLS" (rewriteUrls in JS, --rewrite-urls on command line).
2. Alias --relative-urls as --rewrite-urls=relative (or --rewrite-urls=less-relative?) and deprecate --relative-urls (but keep around at least through next major version)
3. --rewrite-urls would then be: "false" (default), "relative", "modules"

"modules" would apply the folder-relative rewriting when the ./ is present at the beginning of the URL.

That way, we don't get too much option bloat, and it would reduce the confusion between relative-urls and module url rewriting. Basically, these are all just URL rewriting rules (or "modes"), and it doesn't make sense to have two options for URL rewriting that you have to use in some special combination. That would also allow for future expansion if there was a different type of rewriting needed, such as relative to node_modules folder, or whatever it might be (like --rewrite-urls=node).

What do you think?

[markdalgleish]

@matthew-dean that sounds great to me 👍

[markdalgleish]

@matthew-dean sorry for the nudge, but has there been any movement on this?

[matthew-dean]

@seven-phases-max Are you on board?
If so, @sokra Can you update your PR to what I recommended?

[seven-phases-max]

--rewrite-urls would then be: "false" (default), "relative", "modules"

+1. My only objection is the word "module(s)". It does not have any direct connection to ./ thing and therefore does not seem to be self-documenting/evident (not counting the term "CSS Module" refers to something very different with W3C...). Ideally it should be some other keyword (no idea which one yet to use though).

[sokra]

I'm currently on vacation...

[moroshko]

Any update on this PR?

[matthew-dean]

I think we need some better keywords a la @seven-phases-max's suggestions. I'm not familiar enough this usage to suggest anything.

[mmakarin]

Still broken. Common. It's 2016!

[matthew-dean]

@mmakarin Would you like to update and make a new PR?

[jhnns]

I think, --relative-urls=explicit sounds like the best suggestion so far. It is not a breaking change, because --relative-urls defaults to "all" which means: all urls should be treated as relative urls and thus be rewritten. "explicit", on the other hand, means that only explicit relative urls should be rewritten.

In the webpack environment, we usually need to distinguish between relative urls/imports and "module" urls/imports. That's why we had to introduce the special ~ prefix; just to get this separation. But this makes sharing code on npm harder because no one wants to publish a LESS module that is only compatible with webpack.

Tbh: I think --rewrite-urls would be a much better name for this option, because it says what it actually does. But I don't think that this needs to be changed now. And btw: This is one of LESS' killer features 👍. It still bugs me when I'm using SASS 😞

[matthew-dean]

Reminder on this thread. A PR would be merged once #2615 (comment) and #2615 (comment) are addressed. @sokra can you modify the original PR? Or does someone want to create a new PR with these fixes?

[jhnns]

@matthew-dean @seven-phases-max I would be up to implement this

I'd follow @matthew-dean 's suggestion:

1. Deprecate --relative-urls
2. Introduce --rewrite-urls. If present, it comes with two modes:
   • all (default): Rewrite all urls
   • relative: Only rewrite urls that are explicitly relative. You'd use that option in conjunction with CSS modules
3. The deprecated option --relative-urls defaults to --rewrite-urls=all

Should this be done on the current 3.0.0 branch? Would it be ok then to just replace --relative-urls with --rewrite-urls without deprecating it since it is a breaking change? And when do you plan to release 3.0.0? (sorry for bothering you) 😁

[matthew-dean]

@jhnns Go for it! And yes, it's fine on the 3.0 branch. I'm still working through the 3.0 checklist.

[jhnns]

done(). Please review #3041 😁

I also renamed relative to explicit-relative to make things more clear.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.