Fixes #1880 - Adds two new math modes and deprecates strictMath #3274

matthew-dean · 2018-07-07T06:50:57Z

This PR deprecates strictMath in favor of 4 math modes: always | parens-division | parens | strict | strict-legacy

In talking with @calvinjuarez, we both found the current behavior of strictMath extremely odd in cases like width: (1 + 1) + (1 + 1); which would result in the unexpected width: (1 + 1) + (1 + 1); vs. the expected width: 2 + 2;. None of the documentation seems to indicate this behavior or why it would need to evaluate in this way, since strictMath: true will evaluate width: (1 + 1) as width: 2. However, since there were many tests expecting this behavior in how Less deals with parens, we opted to leave it as "legacy" (strict-legacy) and add a more intuitive parens / strict option.

So, in summary, here are the math options (please see tests):

always (current default) - Less eagerly does math
parens-division (future default) - No division is performed outside of parens using / operator (but can be "forced" outside of parens with ./ operator)
parens | strict - A more intuitive form of legacy strictMath: true
strict-legacy (deprecated) - As named, operates exactly like current strictMath: true, with the exception that width: -(1); (single dimension values in parens) will now output width: -1; vs previous behavior of width: -(1)

Note, that setting options.strictMath = true or --strict-math=on is detected and seamlessly upgraded to math: 'strict-legacy' with a warning.

# Conflicts: # lib/less/tree/declaration.js # test/index.js

matthew-dean · 2018-07-07T07:12:12Z

🤔 Should parens-all just be parens ?

calvinjuarez

On parens-all or parens, I think parens is probably sufficient. Otherwise, I've only really got a couple questions.

calvinjuarez · 2018-07-07T23:14:44Z

bin/lessc

@@ -481,8 +482,17 @@ function processPluginQueue() {
                break;
            case 'sm':
            case 'strict-math':
+                console.warning('Strict math is deprecated. Use --math');


This warning could specify --math=parens, maybe.

Fair, yes. Will change it.

calvinjuarez · 2018-07-07T23:21:44Z

lib/less-browser/add-default-options.js

-    // TODO: deprecate and remove 'inlineJavaScript' thing - where it came from at all?
-    options.javascriptEnabled = (options.javascriptEnabled || options.inlineJavaScript) ? true : false;
-
+    options.javascriptEnabled = options.javascriptEnabled ? true : false;


? true : false isn't really necessary is it? Or is this to force Boolean? Do we have a style guide covering this case? Maybe just Boolean(options.javascriptEnabled) would be more straightforward?

I should just remove this line. I was just removing the inlineJavaScript thing, which was never used and shouldn't have gone in.

calvinjuarez · 2018-07-07T23:24:01Z

lib/less-node/lessc-helper.js

+        console.log('  -m=, --math=');
+        console.log('     always                    Less will eagerly perform math operations always.');
+        console.log('     parens-division           Math performed except for division (/) operator');
+        console.log('     parens-all                Math only performed inside parentheses');


As I mentioned in a separate comment, I think parens is probably sufficient, but I don't mind either way, really.

I'll do parens with strict as an alias.

calvinjuarez · 2018-07-07T23:28:00Z

lib/less/math-constants.js

+    ALWAYS: 0,
+    PARENS_DIVISION: 1,
+    PARENS_ALL: 2,
+    STRICT_LEGACY: 3


Speaking of style guides, you have any strong opinion on comma dangle?

Comma dangles shouldn't go in, probably, for compatibility reasons.

calvinjuarez · 2018-07-07T23:30:53Z

lib/less/parser/parser.js

@@ -1920,7 +1920,7 @@ var Parser = function Parser(context, imports, fileInfo) {

                        parserInput.save();

-                        op = parserInput.$char('/') || parserInput.$char('*');
+                        op = parserInput.$char('/') || parserInput.$char('*') || parserInput.$str('./');


I love the ./ thing, by the way.

calvinjuarez · 2018-07-07T23:39:28Z

lib/less/utils.js

+                case 'parens-division':
+                    opts.math = MATH.PARENS_DIVISION;
+                    break;
+                case 'parens-all':


We had talked about strict as an alias of parens/parens-all. I don't think it's totally necessary; I'm just making sure it was a conscious choice to leave it out.

It was, but after sleeping on it, I realized it might make the concept easier for people switching to decide which one to use.

calvinjuarez · 2018-07-07T23:43:44Z

test/index.js

+    }, 'math/parens-all/'],
+    [{
+        math: 'parens-division'
+    }, 'math/parens-division/'],
    [{strictMath: true, strictUnits: true, javascriptEnabled: true}, 'errors/',


Should the strictMath option be updated here (and in other lines below) as well? Or will it not matter?

I left some in to verify that setting the legacy option worked as expected. I should make a note of it though.

- Merge branch 'master' into strict-math-division

calvinjuarez

LGTM!

# Conflicts: # test/less/functions.less

--strict-math has been deprecated and replaced by --math. See less/less.js#3274

@matthew-dean

* package – update css-compile script to use --math --strict-math has been deprecated and replaced by --math. See less/less.js#3274 * maps – begin converting to DRs * each – use each() for loops (WIP) Still haven't updated for the grid breakpoints just yet, though some breakpoint loops have been written in anticipation of those updates. * breakpoints – fix breakpoint JS after converting to DR maps * each – replace all #each-* loop with each or #for-* There are 4 loops in mixins/_grid-framework.less that loop 'til a static number. I'm not aware of any answer for that at the moment, but a plugin could be easily written that could just leverage `each()`. * dev, meta – steal nice stuff from #12 Thanks @matthew-dean! * plugins – fix lint warnings/errors Except one in breakpoints.js: ``` 91:6 warning Variable 'nextBreakpoint' should be initialized on declaration ``` * _tables – fix .table-responsive output order See less/less.js#3340 * _navbar – fix .navbar-expand output order See less/less.js#3340 * README – update to match true min version required Co-Authored-By: calvinjuarez <calvin.juarez@gmail.com> * plugins/theme-color-level – more readable code Co-Authored-By: calvinjuarez <calvin.juarez@gmail.com> * README – replace the other, less easy ways of installing * _grid-framework – restore comment See #14 (comment) * README – wording edits Co-Authored-By: calvinjuarez <calvin.juarez@gmail.com> * _functions – restore commented Sass * README – more wording edits Co-Authored-By: calvinjuarez <calvin.juarez@gmail.com> * README – remove clone note per #14 (comment) * plugins/breakpoints – remove parseUnit(); fix nextBreakpoint ESLint issue See https://github.com/seanCodes/bootstrap-less-port/pull/14/files/94a03d156dde954caccf58130ebc8fa3adaf397e#r240023403 * rename plugin `keys` → `map-keys` Reverting the name change for now, until @calvinjuarez is able to pull in the plugin from NPM and alias it. (See [his comment](#14 (comment) sion_r241965904).) This change addresses [this PR review comment](#14 (comment) sion_r239916331). * _grid-framework – revert loop renaming Reduce the number of changes in the PR to avoid potential bugs. This change addresses [this PR review comment](#14 (comment)). * _transition – revert var renaming Match var names used in the Sass version of the mixin. This change addresses [this PR review comment](#14 (comment)). * breakpoints – put long comments on own line (instead of including at the end of a line) * restore color-function plugins Revert deletion of plugin files that add the `color()`, `gray()` and `theme-color()` functions. Even though Less’ new ruleset accessors could be used, we’re going to keep them for parity with the Sass version and for backwards compatibility. This change addresses [this PR review comment](#14). * restore usages of color functions Revert the conversion the `color()`, `gray()` and `theme-color()` functions to Less’ ruleset accessors. This change is primarily being made to keep the syntax as similar to the Sass version as possible, for easy reference/comparison. Note that keeping things similar to Sass may prove unnecessary in the future, so we may go back to accessors, but for now we’ll keep it like this. This change addresses [this PR review comment](#14). * change color functions to handle rulesets vs lists * fix bug in ruleset-to-map conversion Instead of pulling the `value` directly from each item in the ruleset, evaluate the item first to determine the true value of the item. This fixes the issue where the `value` is actually a var name and what we want is the the value of the var—not the var name itself.

@matthew-dean

* package – update css-compile script to use --math --strict-math has been deprecated and replaced by --math. See less/less.js#3274 * maps – begin converting to DRs * each – use each() for loops (WIP) Still haven't updated for the grid breakpoints just yet, though some breakpoint loops have been written in anticipation of those updates. * breakpoints – fix breakpoint JS after converting to DR maps * each – replace all #each-* loop with each or #for-* There are 4 loops in mixins/_grid-framework.less that loop 'til a static number. I'm not aware of any answer for that at the moment, but a plugin could be easily written that could just leverage `each()`. * dev, meta – steal nice stuff from #12 Thanks @matthew-dean! * plugins – fix lint warnings/errors Except one in breakpoints.js: ``` 91:6 warning Variable 'nextBreakpoint' should be initialized on declaration ``` * _tables – fix .table-responsive output order See less/less.js#3340 * _navbar – fix .navbar-expand output order See less/less.js#3340 * README – update to match true min version required Co-Authored-By: calvinjuarez <calvin.juarez@gmail.com> * plugins/theme-color-level – more readable code Co-Authored-By: calvinjuarez <calvin.juarez@gmail.com> * README – replace the other, less easy ways of installing * _grid-framework – restore comment See #14 (comment) * README – wording edits Co-Authored-By: calvinjuarez <calvin.juarez@gmail.com> * _functions – restore commented Sass * README – more wording edits Co-Authored-By: calvinjuarez <calvin.juarez@gmail.com> * README – remove clone note per #14 (comment) * plugins/breakpoints – remove parseUnit(); fix nextBreakpoint ESLint issue See https://github.com/seanCodes/bootstrap-less-port/pull/14/files/94a03d156dde954caccf58130ebc8fa3adaf397e#r240023403 * rename plugin `keys` → `map-keys` Reverting the name change for now, until @calvinjuarez is able to pull in the plugin from NPM and alias it. (See [his comment](#14 (comment) sion_r241965904).) This change addresses [this PR review comment](#14 (comment) sion_r239916331). * _grid-framework – revert loop renaming Reduce the number of changes in the PR to avoid potential bugs. This change addresses [this PR review comment](#14 (comment)). * _transition – revert var renaming Match var names used in the Sass version of the mixin. This change addresses [this PR review comment](#14 (comment)). * breakpoints – put long comments on own line (instead of including at the end of a line) * restore color-function plugins Revert deletion of plugin files that add the `color()`, `gray()` and `theme-color()` functions. Even though Less’ new ruleset accessors could be used, we’re going to keep them for parity with the Sass version and for backwards compatibility. This change addresses [this PR review comment](#14). * restore usages of color functions Revert the conversion the `color()`, `gray()` and `theme-color()` functions to Less’ ruleset accessors. This change is primarily being made to keep the syntax as similar to the Sass version as possible, for easy reference/comparison. Note that keeping things similar to Sass may prove unnecessary in the future, so we may go back to accessors, but for now we’ll keep it like this. This change addresses [this PR review comment](#14). * change color functions to handle rulesets vs lists * fix bug in ruleset-to-map conversion Instead of pulling the `value` directly from each item in the ruleset, evaluate the item first to determine the true value of the item. This fixes the issue where the `value` is actually a var name and what we want is the the value of the var—not the var name itself.

matthew-dean added 3 commits June 24, 2018 16:24

WIP - Added strictMath: 'division' option

f9ab84a

Merge branch 'master' into strict-math-division

d5f669e

# Conflicts: # lib/less/tree/declaration.js # test/index.js

Fixes less#1880 - Adds two new math modes and deprecates strictMath

76c1034

matthew-dean mentioned this pull request Jul 7, 2018

--math AKA align strict math behavior with documentation. #3273

Closed

matthew-dean requested a review from calvinjuarez July 7, 2018 06:57

calvinjuarez reviewed Jul 7, 2018

View reviewed changes

Change "parens-all" to "parens|strict"

7f9435f

- Merge branch 'master' into strict-math-division

matthew-dean mentioned this pull request Jul 8, 2018

Feature/rewrite urls #3248

Merged

Remove javascriptEnabled from browser options check, add to defaults

556ea61

calvinjuarez approved these changes Jul 11, 2018

View reviewed changes

Merge branch 'master' into strict-math-division

78c38e4

# Conflicts: # test/less/functions.less

matthew-dean merged commit 8938e98 into less:master Jul 11, 2018

calvinjuarez added a commit to calvinjuarez/bootstrap-less-port that referenced this pull request Jul 16, 2018

package – update css-compile script to use --math

ef6d111

--strict-math has been deprecated and replaced by --math. See less/less.js#3274

JamesMcIntosh mentioned this pull request Oct 16, 2018

Current version of less breaks media query aspect ratios jonycheung/deadsimple-less-watch-compiler#69

Closed

calvinjuarez added a commit to calvinjuarez/bootstrap-less-port that referenced this pull request Dec 6, 2018

package – update css-compile script to use --math

c12f2be

--strict-math has been deprecated and replaced by --math. See less/less.js#3274

arichiardi mentioned this pull request Dec 26, 2020

[Build] Not LESS 4 compatible fomantic/Fomantic-UI#1818

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #1880 - Adds two new math modes and deprecates strictMath #3274

Fixes #1880 - Adds two new math modes and deprecates strictMath #3274

matthew-dean commented Jul 7, 2018 •

edited

Loading

matthew-dean commented Jul 7, 2018

calvinjuarez left a comment •

edited

Loading

calvinjuarez Jul 7, 2018

matthew-dean Jul 8, 2018

calvinjuarez Jul 7, 2018

matthew-dean Jul 8, 2018

calvinjuarez Jul 7, 2018

matthew-dean Jul 8, 2018

calvinjuarez Jul 7, 2018

matthew-dean Jul 8, 2018

calvinjuarez Jul 7, 2018

matthew-dean Jul 8, 2018

calvinjuarez Jul 7, 2018

matthew-dean Jul 8, 2018

calvinjuarez Jul 7, 2018

matthew-dean Jul 8, 2018

calvinjuarez left a comment

Fixes #1880 - Adds two new math modes and deprecates strictMath #3274

Fixes #1880 - Adds two new math modes and deprecates strictMath #3274

Conversation

matthew-dean commented Jul 7, 2018 • edited Loading

matthew-dean commented Jul 7, 2018

calvinjuarez left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calvinjuarez left a comment

Choose a reason for hiding this comment

matthew-dean commented Jul 7, 2018 •

edited

Loading

calvinjuarez left a comment •

edited

Loading