Feature rewrite-urls #3041

jhnns · 2017-03-28T01:38:37Z

This PR is based on the discussion #2615. It introduces a new option called rewrite-urls that can have two possible values:

all: Rewrite all url() statements to the base less file
explicit-relative: Only rewrite url() statements that start with a .. This enables CSS modules support

The option is disabled by default. The option relative-urls is superseded by rewrite-urls=all.

As a side-effect, this PR also introduces a second change: Explicit relative URLs stay explicit relative – irregardless of the rewrite-url option. Check out the changes in test/css/static-urls/urls.css and test/css/urls.css.

- Rename isPathRelative to pathRequiresRewrite - Add special handling for rewriteUrls=relative - Add rewritePath for path rewriting - Ensure that explicit relative paths stay explicit relative - Remove code style inconsistencies

jhnns · 2017-03-28T01:39:45Z

lib/less/contexts.js

+    return newPath;
+};
+
+contexts.Eval.prototype.normalizePath = function (path) {


Changes in normalizePath are just code style issues

jhnns · 2017-03-28T01:54:29Z

Doesn't look like CI fails are related to these changes...

seven-phases-max · 2017-03-28T07:32:10Z

👍
Just a few (minor) thoughts:

Ideally there should be an explicit value/flag to turn it off (not just "don't touch me and I'm off by default"), e.g.: rewrite-urls=off|all|explicit-relative (relative-urls did not have this in lessc, but via JS we can set it as options.relativeUrls = false)
explicit-relative: honestly I can't see the verbosity makes it either more evident or self-documented.... Just dot or something would be in the same league of magic spells (One can guess neither meaning w/o consulting a corresponding docs section anyway :).

jhnns · 2017-03-28T10:18:12Z

Ideally there should be an explicit value/flag to turn it off (not just "don't touch me and I'm off by default"), e.g.: rewrite-urls=off|all|explicit-relative (relative-urls did not have this in lessc, but via JS we can set it as options.relativeUrls = false)

I can add that.

explicit-relative: honestly I can't see the verbosity makes it either more evident or self-documented.... Just dot or something would be in the same league of magic spells (One can guess neither meaning w/o consulting a corresponding docs section anyway :).

I do understand. I just felt like relative would not be ok since paths not starting with a dot are also relative in CSS. Maybe it's better to refer to css-modules explicitly? It is well defined what the meaning of paths are in CSS modules. In this case, the option value would refer to the rewriting "mode".

Then we would have: off|all|css-modules?

jhnns · 2017-03-28T11:56:40Z

Or is css-modules too verbose? We could also use rewrite-urls=dot which is equally descriptive ^^. I agree that none of these solutions is self-describing 😞

seven-phases-max · 2017-03-28T17:16:47Z

css-modules would be as misleading as microsoft word. (Neither the css-modules project nor W3C CSS Module(s) have anything to do with ./... Or in other words, realize the css-modules is not the only project benefits from the feature. The new feature also improves things for a project named ferljaanien-quorliyanien - shall I continue? :)

matthew-dean · 2017-03-28T17:52:28Z

I'm also not a fan of explicit-relative. I also didn't notice that you had removed the default "off"/false option in the previous thread, so that's really not good. off|modules|relative or off|local|all or off|local|relative|all something like that is needed. (Do we need an option to rewrite "relative" URLs but NOT dot-relative (local-relative) URLs? If so, 4 options would be needed.)

So, it sounds like there's still a problem on the naming/semantics front.

seven-phases-max · 2017-03-28T20:19:26Z

Do we need an option to rewrite "relative" URLs but NOT dot-relative (local-relative) URLs?

I think no, we don't. But I'm afraid by now we might already have some drift in expectations of which option is doing what exactly... Probably a table of results of every option for every path ("abs://foo", "../foo", "./foo", "foo") would be helpful.

jhnns · 2017-03-29T10:06:18Z

Do we need an option to rewrite "relative" URLs but NOT dot-relative (local-relative) URLs?

I can't think of a use-case.

I really like off|local|all, don't know why. Maybe it's because that in the command line ./cmd is always local, where as cmd will also lookup the $PATH (just like CommonJS modules).

If you agree on off|local|all, I'll add this to the PR.

Probably a table of results of every option for every path ("abs://foo", "../foo", "./foo", "foo") would be helpful.

Yes, especially in conjunction with the root-path option which will always rewrite the url.

matthew-dean · 2017-10-09T03:12:06Z

What's the status of this?

jhnns · 2017-10-09T15:49:22Z

I was waiting for feedback on the off|local|all option :)

matthew-dean · 2017-10-10T05:50:12Z

I think we just need weigh-in from @seven-phases-max. Then hopefully you could proceed and merge this (although there are a few conflicts now).

stale · 2018-06-07T12:27:03Z

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.

matthew-dean · 2018-06-16T19:00:19Z

Pinging @less/core again for this.

seven-phases-max · 2018-06-16T22:53:23Z

Well, I put 4.0 label since the way it is it can't be released as 3.x anyway (as it's breaking). Other than that I can't see any problems merging this (if the conflicts are resolved of course).

matthew-dean · 2018-06-18T07:43:24Z

@seven-phases-max 🤔 Hmm... I'm not certain it's breaking. It seems to have appropriate backwards-compatibility fallbacks, but as the task runners were changed, it's hard to verify that it ISN'T breaking.

That is, lines like this suggest it is a breaking change, but it would be better to mix the two i.e. show that rewriteUrls = true still works (but add deprecation comment), and change some runners with the new option.

So, yeah, maybe some clarification on this PR (and tests demo-ing both feature implementation and backwards compatibility) would be helpful.

matthew-dean · 2018-06-18T19:34:52Z

@jhnns Are you still able to resolve this and answer the above questions?

jhnns · 2018-06-20T09:03:04Z

Yes, I can work on this the next days 👍

Regarding your questions: I have to look into it again, but I think it's not a breaking change as the old option is still available as rewrite-urls=all. When resolving the merge conflicts, I can specifically try keep it backwards compatible (and yes, I'll re-add the old tests).

So, I'll use off|local|all as possible values for rewrite-urls, ok?

seven-phases-max · 2018-06-20T09:52:58Z

It seems to have appropriate backwards-compatibility fallbacks,

Sure I could miss something. So... if some external code passes options.relativeUrls = true in, will the result be identical to what it is now?

- Rename explicit-relative to local - Add rewrite-urls argument validation - Replace unknown CSS property background-url with background-image Based on discussion less#3041

- Remove redundant tests - Add rootpath-rewrite-urls-all test - Add rootpath-rewrite-urls-local test

jhnns · 2018-06-26T12:34:33Z

lib/less-rhino/index.js

    }

    var j = file.lastIndexOf('/');
-    if (newFileInfo.relativeUrls && !/^(?:[a-z-]+:|\/)/.test(file) && j != -1) {
+    if (newFileInfo.rewriteUrls && !/^(?:[a-z-]+:|\/)/.test(file) && j != -1) {


Not sure if we need to change that part. However, for me it looks like less.Parser.fileLoader is not used anywhere. Is that file even up-to-date?

No, less-rhino should have been pulled in 3.0, but I just wasn't sure. I doubt it even still works. Ideally, PR #2985 should be re-written and pulled into its own repo with less as a dependency (as in https://github.com/less/less-java), and then less-rhino should be removed as a sub-folder.

Long and short, there aren't any active Java maintainers in the Less core team, so we really can't manage it within this repo.

jhnns · 2018-06-26T13:31:48Z

I resolved the merge conflicts, changed the options to all|local|off and added additional tests for the case where the rootpath and the rewrite-urls option is mixed.

The PR is almost done from my point of view. If used from the command line (via bin/lessc) or in the browser, the deprecated relativeUrls option is mapped to rewriteUrls: 'all'. The only problem I currently have is that we also need to ensure backwards compatibility for the Node.js API. I don't know where to put the mapping from relativeUrls to rewiteUrls: 'all'.

I'd appreciate some help here so that we can finally merge that feature :)

matthew-dean · 2018-06-26T17:58:59Z

@jhnns

I'd appreciate some help here so that we can finally merge that feature :)

Thanks for your work in re-working this! I'll see if I can take a look sometime soon, but anyone else is welcome to take this one of course.

* Rename relativeUrls option to rewriteUrls * Refactor contexts.js - Rename isPathRelative to pathRequiresRewrite - Add special handling for rewriteUrls=relative - Add rewritePath for path rewriting - Ensure that explicit relative paths stay explicit relative - Remove code style inconsistencies * Add missing tests for url rewriting * Rename rewrite-urls option value to explicit-relative * Add missing tests for url rewriting * Refactor rewrite urls options - Rename explicit-relative to local - Add rewrite-urls argument validation - Replace unknown CSS property background-url with background-image Based on discussion #3041 * Re-add tests for deprecated relative-urls option * Improve tests - Remove redundant tests - Add rootpath-rewrite-urls-all test - Add rootpath-rewrite-urls-local test * Fix typo in unknown argument warning * Revert old tests to deprecated relativeUrls option again * Added more CSS Grid tests * All tests passing * Merge branch 'master' into feature/rewrite-urls (#3282) * WIP - Added strictMath: 'division' option * Removes `less-rhino` (broken for a long time) - Fixes #3241 * Expressions require a delimiter of some kind in mixin args * Removes "double paren" issue for boolean / if function * WIP each() re-implementation * Allows plain conditions without parentheses * Added each() and tests * Added tests calling mixins from each() * Fixes #1880 - Adds two new math modes and deprecates strictMath * Allows named args for detached ruleset (anonymous mixin) * Makes sure this doesn't regress needing parens around mixin guard conditions * Remove javascriptEnabled from browser options check, add to defaults * Workaround weird Jasmine conversion of < to HTML entity * Removes remaining Rhino cruft * Remove rhino files from bower * Bump Jasmine version * Added .() example to each * v3.6.0 * Update CHANGELOG.md * add explicit nested anonymous mixin test * add explicit nested anonymous mixin test result * derp: align test with expected output * v3.7.0 (#3279) * Update CHANGELOG.md * Rewrite the rewriteUrls feature to use same pattern as strictMath-to-math * Update console warning for --relative-urls

jhnns added 4 commits March 27, 2017 19:06

Rename relativeUrls option to rewriteUrls

cb4a577

Refactor contexts.js

b95c294

- Rename isPathRelative to pathRequiresRewrite - Add special handling for rewriteUrls=relative - Add rewritePath for path rewriting - Ensure that explicit relative paths stay explicit relative - Remove code style inconsistencies

Add missing tests for url rewriting

99df16a

Rename rewrite-urls option value to explicit-relative

e80a91d

jhnns commented Mar 28, 2017

View reviewed changes

Add missing tests for url rewriting

bfc7812

jhnns mentioned this pull request Mar 28, 2017

add moduleUrl option #2615

Closed

seven-phases-max force-pushed the 3.x branch from 4483517 to 3f353a8 Compare June 8, 2017 06:45

stale bot added the stale label Feb 7, 2018

less deleted a comment from stale bot Feb 7, 2018

stale bot removed the stale label Feb 7, 2018

stale bot added the stale label Jun 7, 2018

seven-phases-max added 4.0 and removed stale labels Jun 7, 2018

matthew-dean removed the 4.0 label Jun 25, 2018

matthew-dean added this to the 4.0 milestone Jun 25, 2018

jhnns added 4 commits June 26, 2018 11:13

Merge remote-tracking branch 'upstream/master' into feature/rewrite-urls

5ede205

Refactor rewrite urls options

c2eea6b

- Rename explicit-relative to local - Add rewrite-urls argument validation - Replace unknown CSS property background-url with background-image Based on discussion less#3041

Re-add tests for deprecated relative-urls option

78b5254

Improve tests

2e73a02

- Remove redundant tests - Add rootpath-rewrite-urls-all test - Add rootpath-rewrite-urls-local test

jhnns changed the base branch from 3.x to master June 26, 2018 12:30

Fix typo in unknown argument warning

2a4dfec

jhnns force-pushed the feature/rewrite-urls branch from c8f65df to 2a4dfec Compare June 26, 2018 12:33

jhnns commented Jun 26, 2018

View reviewed changes

Revert old tests to deprecated relativeUrls option again

e0b7a6b

matthew-dean closed this Jun 30, 2018

matthew-dean mentioned this pull request Jun 30, 2018

Feature/rewrite urls #3248

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature rewrite-urls #3041

Feature rewrite-urls #3041

jhnns commented Mar 28, 2017

jhnns Mar 28, 2017

jhnns commented Mar 28, 2017

seven-phases-max commented Mar 28, 2017

jhnns commented Mar 28, 2017 •

edited

Loading

jhnns commented Mar 28, 2017 •

edited

Loading

seven-phases-max commented Mar 28, 2017 •

edited

Loading

matthew-dean commented Mar 28, 2017

seven-phases-max commented Mar 28, 2017 •

edited

Loading

jhnns commented Mar 29, 2017 •

edited

Loading

matthew-dean commented Oct 9, 2017

jhnns commented Oct 9, 2017

matthew-dean commented Oct 10, 2017

stale bot commented Jun 7, 2018

matthew-dean commented Jun 16, 2018

seven-phases-max commented Jun 16, 2018

matthew-dean commented Jun 18, 2018

matthew-dean commented Jun 18, 2018

jhnns commented Jun 20, 2018

seven-phases-max commented Jun 20, 2018 •

edited

Loading

jhnns Jun 26, 2018

matthew-dean Jun 26, 2018

jhnns commented Jun 26, 2018

matthew-dean commented Jun 26, 2018 •

edited

Loading

Feature rewrite-urls #3041

Feature rewrite-urls #3041

Conversation

jhnns commented Mar 28, 2017

jhnns Mar 28, 2017

Choose a reason for hiding this comment

jhnns commented Mar 28, 2017

seven-phases-max commented Mar 28, 2017

jhnns commented Mar 28, 2017 • edited Loading

jhnns commented Mar 28, 2017 • edited Loading

seven-phases-max commented Mar 28, 2017 • edited Loading

matthew-dean commented Mar 28, 2017

seven-phases-max commented Mar 28, 2017 • edited Loading

jhnns commented Mar 29, 2017 • edited Loading

matthew-dean commented Oct 9, 2017

jhnns commented Oct 9, 2017

matthew-dean commented Oct 10, 2017

stale bot commented Jun 7, 2018

matthew-dean commented Jun 16, 2018

seven-phases-max commented Jun 16, 2018

matthew-dean commented Jun 18, 2018

matthew-dean commented Jun 18, 2018

jhnns commented Jun 20, 2018

seven-phases-max commented Jun 20, 2018 • edited Loading

jhnns Jun 26, 2018

Choose a reason for hiding this comment

matthew-dean Jun 26, 2018

Choose a reason for hiding this comment

jhnns commented Jun 26, 2018

matthew-dean commented Jun 26, 2018 • edited Loading

jhnns commented Mar 28, 2017 •

edited

Loading

jhnns commented Mar 28, 2017 •

edited

Loading

seven-phases-max commented Mar 28, 2017 •

edited

Loading

seven-phases-max commented Mar 28, 2017 •

edited

Loading

jhnns commented Mar 29, 2017 •

edited

Loading

seven-phases-max commented Jun 20, 2018 •

edited

Loading

matthew-dean commented Jun 26, 2018 •

edited

Loading