Feature/rewrite urls #3248

matthew-dean · 2018-06-30T22:45:02Z

Moved from #3041

- 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

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

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

matthew-dean · 2018-06-30T22:46:43Z

I closed this and re-opened a PR with fixed tests. See: #3248.

Are there any error cases missing? Can rewrite URLs cause an error?

If this looks good, we can merge. Thanks @jhnns!

calvinjuarez · 2018-07-07T02:53:33Z

Tests failing. The failure is the same as some other PRs.

jasmine:rootpathRewriteUrls
[secure].js browser test - rootpath and rewrite urls
   - original-[secure]:test-[secure]-urls should match the expected output......X
     ReferenceError: Can't find variable: Promise in http://localhost:8081/test/browser/common.js (line 252) (1)

matthew-dean · 2018-07-07T07:19:57Z

@calvinjuarez I think this is something wrong with our tests rather than the PR, since it happens everywhere and also happens randomly (and doesn't happen for Appveyor). Maybe some polyfill is loading asynchronously in Node 9 for PhantomJS?

matthew-dean · 2018-07-08T02:39:23Z

This should probably wait until #3274 is merged and then do the "options upgrade" the same way.

* 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 * Update CHANGELOG.md * Update CHANGELOG.md

# Conflicts: # bin/lessc # lib/less-browser/add-default-options.js # lib/less-node/lessc-helper.js # lib/less/parse.js # test/index.js # test/less/math/parens-division/parens.less # test/less/math/strict/parens.less # test/less/strict-math/parens.less

…math

matthew-dean · 2018-07-11T22:07:05Z

I redid this branch to use the same "upgrade" pattern as the strictMath-to-math upgrade pattern.

That is, any options sent to parse() or render() are translated from string values to constants. I'm not sure it'll make a huge difference in evaluation time, but string comparisons during evaluation for options seemed unnecessary. Ideally all options would be either integer checks or booleans.

# Conflicts: # bin/lessc

matthew-dean · 2018-07-11T22:21:40Z

@less/core

Since I don't have a firm grasp of this feature, I think it the tests should be reviewed by @jhnns and others before this is merged to see if the input/output is intuitive and as expected. I didn't really verify the feature; I just got it working / synced with master.

calvinjuarez

I'm not sure I'm fully qualified to pass this one off, since I haven't been following the issue, but I'll work my way through the code. For now, just a couple questions/thoughts.

We could add tests for the deprecation warning to the Grunt opts test.

Gruntfile.js @ ln. 204

'node bin/lessc --relative-urls test/less/lazy-eval.less tmp/lazy-eval.css',

bin/lessc

@@ -4,7 +4,7 @@ var path = require('path'),
    fs = require('../lib/less-node/fs'),
    os = require('os'),
    utils = require('../lib/less/utils'),
-    MATH = require('../lib/less/math-constants'),
+    Constants = require('../lib/less/constants'),


calvinjuarez · 2018-07-11T22:16:58Z

bin/lessc

            case 'relative-urls':
-                options.relativeUrls = true;
+                console.warn('The relative-urls option has been replaced by the rewrite-urls option and will be removed.');


Consider rephrasing to match other deprecation warnings?

The --relative-urls option has been deprecated. Use --rewrite-urls=all.

(I'm not entirely sure which rewrite-urls corresponds to relative-urls.)

jhnns · 2018-07-18T15:51:06Z

Looks good to me 👍 :)

matthew-dean · 2018-07-18T19:36:00Z

Thanks @jhnns!

matthew-dean · 2018-07-18T19:38:54Z

@jhnns Would you be willing to write up documentation in a pull request for https://github.com/less/less-docs? You can see an example of option deprecation for the new math setting: http://lesscss.org/usage/#less-options-math

jhnns · 2018-09-30T12:31:42Z

Sure, I can do that :)

Based on less/less.js#3248

jhnns · 2018-09-30T14:07:22Z

https://github.com/less/less-docs/pull/491/files

jhnns and others added 16 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

Add missing tests for url rewriting

bfc7812

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 #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

Fix typo in unknown argument warning

2a4dfec

Revert old tests to deprecated relativeUrls option again

e0b7a6b

Added more CSS Grid tests

b94f041

Merge branch 'master' of https://github.com/less/less.js

3a2f805

Merge branch 'master' of https://github.com/less/less.js

f11f8b0

Merge branch 'master' into feature/rewrite-urls

aa376d8

All tests passing

a08c434

matthew-dean requested a review from seven-phases-max June 30, 2018 22:47

Merge branch 'master' into feature/rewrite-urls

0246ade

Merge branch 'master' into feature/rewrite-urls

f0ab5be

matthew-dean added the don't merge label Jul 8, 2018

calvinjuarez and others added 3 commits July 11, 2018 14:32

Rewrite the rewriteUrls feature to use same pattern as strictMath-to-…

6e75e33

…math

matthew-dean removed the don't merge label Jul 11, 2018

matthew-dean requested a review from calvinjuarez July 11, 2018 22:04

matthew-dean mentioned this pull request Jul 11, 2018

Fix #3281: console.warning → console.warn #3283

Merged

Merge branch 'master' into feature/rewrite-urls

07fd5be

# Conflicts: # bin/lessc

calvinjuarez reviewed Jul 11, 2018

View reviewed changes

Update console warning for --relative-urls

8b63b49

matthew-dean merged commit 2d43c0b into master Jul 18, 2018

matthew-dean mentioned this pull request Jul 29, 2018

data-uri function uses path of data-uri call, not the string with the path to the file in. #2629

Open

jhnns added a commit to jhnns/less-docs that referenced this pull request Sep 30, 2018

Add rewriteUrls option

2aee8a6

Based on less/less.js#3248

jhnns mentioned this pull request Sep 30, 2018

Add rewriteUrls option less/less-docs#491

Merged

jhnns mentioned this pull request Sep 30, 2018

Define a standard for custom URL loaders sass/sass#2535

Open

2 tasks

iChenLei deleted the feature/rewrite-urls branch July 20, 2021 03:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/rewrite urls #3248

Feature/rewrite urls #3248

matthew-dean commented Jun 30, 2018 •

edited

Loading

matthew-dean commented Jun 30, 2018

calvinjuarez commented Jul 7, 2018 •

edited

Loading

matthew-dean commented Jul 7, 2018

matthew-dean commented Jul 8, 2018

matthew-dean commented Jul 11, 2018

matthew-dean commented Jul 11, 2018

calvinjuarez left a comment

This comment was marked as resolved.

calvinjuarez Jul 11, 2018

jhnns commented Jul 18, 2018

matthew-dean commented Jul 18, 2018

matthew-dean commented Jul 18, 2018

jhnns commented Sep 30, 2018

jhnns commented Sep 30, 2018

Feature/rewrite urls #3248

Feature/rewrite urls #3248

Conversation

matthew-dean commented Jun 30, 2018 • edited Loading

matthew-dean commented Jun 30, 2018

calvinjuarez commented Jul 7, 2018 • edited Loading

matthew-dean commented Jul 7, 2018

matthew-dean commented Jul 8, 2018

matthew-dean commented Jul 11, 2018

matthew-dean commented Jul 11, 2018

calvinjuarez left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

calvinjuarez Jul 11, 2018

Choose a reason for hiding this comment

jhnns commented Jul 18, 2018

matthew-dean commented Jul 18, 2018

matthew-dean commented Jul 18, 2018

jhnns commented Sep 30, 2018

jhnns commented Sep 30, 2018

matthew-dean commented Jun 30, 2018 •

edited

Loading

calvinjuarez commented Jul 7, 2018 •

edited

Loading