Downgrade the Node.js LESS compiler from v4 to v3 to avoid less compilation problems #38109

serhii-chernenko · 2023-10-20T15:07:23Z

Description (*)

I don't even believe that no one didn't comment this PR #31618 during 2 years. Do you know that styles sourcemaps are broken since updating this package in package.json.
each time it has to be fixed for a new project.
grunt-contrib-less 1.4.1:

grunt-contrib-less ~2.1.0:

you ask for self-checking, tests running, screenshots, but don't check the first development steps when you update something. that's weird.

video example that includes the fix (use subtitles) with timecode:
https://youtu.be/NEKvPRTW9mM?si=2f_ZXV-WNR8IyN9N&t=699

that's even funny that in M2.4.7 beta this package is updated to ~3.0.0
203f415

but it couldn't be compiled because of error:

Running "less:demo" (less) task
>> pub/static/frontend/Vaimo/demo/en_US/css/source/_reset.less: [L7:C4] Error evaluating function `unit`: the first argument to unit must be a number. Have you forgotten parenthesis?
Warning: Error compiling pub/static/frontend/Vaimo/demo/en_US/css/styles-m.less Use --force to continue.

Aborted due to warnings.

Related Pull Requests

Manual testing scenarios (*)

setup a new theme
prepare Grunt files
run npm install
run grunt clean:theme && exec:theme && less:theme
see that sourcemaps not generated even if you disable static sign, cache, browser cache, etc.
restore the package version to 1.4.1 from ~2.1.0
run npm i
repeat the 4th step
see sourcemaps in the browser's devtools.

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
All automated tests passed successfully (all builds are green)

m2-assistant · 2023-10-20T15:07:26Z

Hi @serhii-chernenko. Thank you for your contribution!
Here are some useful tips on how you can test your changes using Magento test environment.

Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests
Semantic Version Checker

You can find more information about the builds here
ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.

For more details, review the Code Contributions documentation.
Join Magento Community Engineering Slack and ask your questions in #github channel.

…3.0.0)

ihor-sviziev · 2023-10-20T17:30:43Z

Hi @serhii-chernenko,
Thank you so much for contributing!

I'm not sure exact reason why it was updated, but guess it was due to security reasons and/or compatibility with newer nodejs versions.

From the fix perspective your fix is fully valid and correct, while from security perspective, having latest version is more secure. So I would prefer fixing the reasoning of this regression rather than downgrading dependencies.

Will you be able to check that?

serhii-chernenko · 2023-10-20T20:24:25Z

hi @ihor-sviziev,
thanks for the answer!

You're right about up-to-dated packages when they used in production or it's an application that built on NPM packages. While the magento 2 uses grunt only for local development, it's not really matter IMHO.

There's just a funny findings:
https://www.npmjs.com/package/grunt-contrib-less

As you can see, the last updates were added 3 years ago. So, I can't say more in the security perspective in this case.

I won't have time to investigate this issue soon. Probably someone else could be able to investigate this issue. If not, you can leave this PR opened, and I'll have a look at this later when I'll have time.

hostep · 2023-10-21T10:43:41Z

@serhii-chernenko: wasn't this already fixed in #28415? This is to be released in Magento 2.4.7, I agree that it took a very long time before it got picked up, but it's what it is I'm afraid.

However, your observation around unit is correct, tested on latest 2.4-develop:

$ grunt less:luma
Running "less:luma" (less) task
>> pub/static/frontend/Magento/luma/en_US/css/source/_reset.less: [L7:C4] Error evaluating function `unit`: the first argument to unit must be a number. Have you forgotten parenthesis?
Warning: Error compiling pub/static/frontend/Magento/luma/en_US/css/styles-m.less Use --force to continue.

Aborted due to warnings.

hostep · 2023-10-21T11:38:25Z

The unit error can be fixed by adding some parenthesis, like this:

diff --git a/lib/web/css/source/lib/variables/_typography.less b/lib/web/css/source/lib/variables/_typography.less
index e357b6969db..177b061323b 100644
--- a/lib/web/css/source/lib/variables/_typography.less
+++ b/lib/web/css/source/lib/variables/_typography.less
@@ -29,10 +29,10 @@
 @font-size-ratio__base: 1.4; // Defines ratio of the root font-size to the base font-size

 @font-size-unit: rem; // The unit to which most typography values will be converted by default
-@font-size-unit-ratio: unit(@root__font-size * 16/100); // Ratio of the root font-size to the font-size unit
+@font-size-unit-ratio: unit((@root__font-size * 16/100)); // Ratio of the root font-size to the font-size unit
 @font-size-unit-convert: true; // Controls whether font-size values are converted to the specified font-size unit

-@font-size__base: unit(@font-size-unit-ratio * @font-size-ratio__base, px); // Base font size value in <b>px</b>
+@font-size__base: unit((@font-size-unit-ratio * @font-size-ratio__base), px); // Base font size value in <b>px</b>
 @font-size__xl: ceil(1.5 * @font-size__base); // 21px
 @font-size__l: ceil(1.25 * @font-size__base); // 18px
 @font-size__s: ceil(.85 * @font-size__base); // 12px

However, after this another less compilation error pops up:

Running "less:luma" (less) task
>> pub/static/frontend/Magento/luma/en_US/Magento_Catalog/css/source/_module.less: [L550:C12] Operation on an invalid type
Warning: Error compiling pub/static/frontend/Magento/luma/en_US/css/styles-m.less Use --force to continue.

But the problems keep coming, after disabling that one line quickly as a test, yet another error comes up:

Running "less:luma" (less) task
>> undefined: [Lundefined:Cundefined] Operation on an invalid type
Warning: Error compiling pub/static/frontend/Magento/luma/en_US/css/styles-m.less Use --force to continue.

So it sounds like the upgrade from less v3 to v4 from a1bbede#diff-0b861b52f90917125f2829df6f171d1248cc83df2a15ff7d7dc7b6f59048e9b1 was not properly tested. Maybe we should revert this partially?
Or go over all broken less code and fix it somehow, but that might take more effort ...

Other than this, we really need some automated tests to verify the nodejs parts of Magento (like compiling less code) works as it should work, I have the feeling we don't have automated tests for this, adding these could prevent errors like this in the future ...

For completion's sake, found it in some comment in the v4.0.0 pull request of less (there is no changelog, no upgrade guide, so this is a bit difficult to deal with):

a) wrap division in parens to indicate you want math performed,
...
The long and short is that / is a valid separator in CSS values, so parsing this as a division operation is problematic (even if division is not performed!)

If I search through magento's less codebase, I find a lot of divisions that aren't in parenthesis, so those will most likely also start causing issues. So downgrading to less v3 sounds like the best plan here for now. Other opinions?

@glo71317: what do you think, since you were involved in AC-8132? Also please in the future always test the nodejs / grunt workflow after updating nodejs dependencies. Thanks!

glo17680 · 2023-10-23T13:31:41Z

@hostep we have upgraded Less js library to the latest version (4.1.3) that time and tested by running tests i.e., grunt spec:luma. Tests was successful. However, it seems like this issue would have been existed previously as well before the recent upgrade. We will look into the below less compilation unit error and will update accordingly.

$ grunt less:luma
Running "less:luma" (less) task
pub/static/frontend/Magento/luma/en_US/css/source/_reset.less: [L7:C4] Error evaluating function `unit`: the first argument to unit must be a number. Have you forgotten parenthesis?
Warning: Error compiling pub/static/frontend/Magento/luma/en_US/css/styles-m.less Use --force to continue.

Aborted due to warnings.

hostep · 2023-10-23T18:20:16Z

The following downgrades make less compilation with grunt work without errors:

diff --git a/package.json.sample b/package.json.sample
index 9fbde04e8a7..faa6cb7ff47 100644
--- a/package.json.sample
+++ b/package.json.sample
@@ -18,7 +18,7 @@
     "grunt-contrib-cssmin": "~4.0.0",
     "grunt-contrib-imagemin": "~4.0.0",
     "grunt-contrib-jasmine": "~4.0.0",
-    "grunt-contrib-less": "~3.0.0",
+    "grunt-contrib-less": "~2.1.0",
     "grunt-contrib-watch": "~1.1.0",
     "grunt-eslint": "~24.2.0",
     "grunt-exec": "~3.0.0",
@@ -27,7 +27,7 @@
     "grunt-template-jasmine-requirejs": "~0.2.3",
     "grunt-text-replace": "~0.4.0",
     "imagemin-svgo": "~9.0.0",
-    "less": "4.1.3",
+    "less": "~3.13.1",
     "load-grunt-config": "~4.0.1",
     "morgan": "~1.10.0",
     "node-minify": "~3.6.0",

@serhii-chernenko: I would suggest you update your PR in this way. Maybe also change the title of the PR (as the sourcemap generation was already fixed in an earlier PR like I already mentioned)

Steps to reproduce:

# to remove any traces of old install
$ rm -R node_modules/ package-lock.json

# make changes to `package.json` file (if it doesn't exist, copy it from `package.json.sample`)

# install nodejs packages
$ npm install

# execute grunt commands (will only work when you've copied `Gruntfile.js.sample` to `Gruntfile.js`)
$ grunt clean:luma && grunt exec:luma && grunt less:luma

ihor-sviziev · 2023-10-23T19:34:08Z

@glo17680 can we submit a PR for downgrading grunt-contrib-less and less as @hostep suggested in #38109 (comment)? Or will you downgrade it internally?

serhii-chernenko · 2023-10-24T11:37:06Z

@hostep but what's the difference with the LTS version 2.4.6-p3?

source maps aren't working there with the same versions:

"grunt-contrib-less": "~2.1.0",
"less": "3.13.1"

hostep · 2023-10-24T12:26:13Z

@serhii-chernenko, please re-read my earlier comment:

@serhii-chernenko: wasn't this already fixed in #28415? This is to be released in Magento 2.4.7, I agree that it took a very long time before it got picked up, but it's what it is I'm afraid.

So issue has been fixed but hasn't been released yet. It will be released in Magento 2.4.7. But if you want a quick fix now, then apply the following changes using a patch: https://github.com/magento/magento2/pull/28415/files

serhii-chernenko · 2023-10-24T12:33:41Z

@hostep oh, I see. sorry, I didn't get that the change was only in 2.4.7. yeah, the package downgrading has to be a solution for 2.4.7 then

hostep · 2023-10-24T16:40:04Z

Great, @serhii-chernenko, do you want to update your PR with my suggested downgrades? Or should we open a new one?

grunt-contrib-less and less packages downgraded to make source maps working for magento 2.4.7+

serhii-chernenko · 2023-10-24T16:44:14Z

@hostep done

ihor-sviziev · 2023-10-25T07:22:10Z

@serhii-chernenko and the last thing - pls sign the Adobe CLA

ihor-sviziev · 2023-10-25T07:23:08Z

@magento run all tests

magento-automated-testing · 2023-10-25T07:23:16Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

serhii-chernenko · 2023-10-27T11:38:54Z

@ihor-sviziev signed

hostep · 2023-10-27T12:43:35Z

I still think we should change the title & description & manual testing steps of this PR to avoid confusion for anyone that runs against this ticket.

The PR is not fixing sourcemaps (because it was already fixed by another PR earlier), it just downgrades the nodejs less compiler from v4 to v3 to avoid less compilation problems.

serhii-chernenko · 2023-10-27T13:39:15Z

@hostep @ihor-sviziev done

glo17680 · 2023-12-05T08:05:39Z

We have fixed and tested the issue with latest LESS library v4.2.0 fe17ac2. It would be available for upcoming release.
The PR 38109 is not fixing sourcemaps just downgrades the nodejs less compiler from v4 to v3 instead. So, it seems that it won't be required after the fe17ac2.

hostep · 2024-01-03T20:08:50Z

@glo17680: I just tried out your changes from 320182b & fe17ac2 and there are still a ton of problems. It feels like those changes haven't been tested by reviewing the frontend rendering...

I just did that and compared the results from grunt less compilation using less.js 3.13.1 to 4.2.0
I can already tell by seeing the homepage or the create new account form of Magento that all kind of sizes are a lot smaller then how they were before.
On the left side less.js v3, on the right side less.js v4. Homepae:

Create new account form:

If I take a look at the differences of the resulting css that gets generated between v3 and v4 I'm seeing about 187 changes of which most have to do with calculating sizes, some extracts:

diff -urwN v3/styles-l.css v4/styles-l.css
--- v3/styles-l.css   2024-01-03 20:48:55
+++ v4/styles-l.css   2024-01-03 20:45:28
@@ -304,7 +304,7 @@
     padding: 0 12px 0 0;
     box-sizing: border-box;
     display: inline-block;
-    width: 50%;
+    width: 100% / 2;
     vertical-align: top;
   }
   .abs-form-field-column-2 .fieldset .field + .fieldset,
@@ -335,7 +335,7 @@
     width: 100%;
   }
   .abs-form-field-revert-column-1 {
-    width: 100%;
+    width: 100% / 1;
   }
   .abs-forms-general-desktop,
   .form.search.advanced,
@@ -378,7 +378,7 @@
   .box-information .box-content,
   .block-balance-giftcard .block-content,
   .block-reviews-dashboard .product-name {
-    font-size: 1.6rem;
+    font-size: 16rem / 10;
   }
   .abs-account-table-margin-desktop,
   .table-wrapper.balance-history,
@@ -506,7 +506,7 @@
   .abs-discount-block-desktop .block > .title strong,
   .paypal-review-discount .block > .title strong,
   .cart-discount .block > .title strong {
-    font-size: 1.6rem;
+    font-size: 16rem / 10;
   }
   .abs-discount-block-desktop .block .content,
   .paypal-review-discount .block .content,
...

The resulting css with v4 is considered invalid by google chrome, see all these exclamation marks:

So I would very strongly recommend we downgrade again to v3, or make a follow up fix before Magento 2.4.7 gets released that fixes all those incorrect calculations being done now in v4 of the less.js compiler ...

Also, I would strongly recommend you figure out a way to properly test these kind of changes for the future, so next time you want to attempt a less.js library upgrade, you'll immediately can see the results and can properly compare the results with how they were before the upgrade.

hostep · 2024-01-07T17:40:49Z

@glo17680: I've spend the entire afternoon today on #38335, but I'm running out of time and energy. The grunt compilation I've tested and it fixes the issue I discussed before. The normal bin/magento setup:static-content:deploy ... command I didn't test because I have no more time today.
Feel free to try it out and give feedback or continue working with my changes because I don't know if I'll find more time/energy later this month to keep working on this in case the PR still contains problems.

andrewbess · 2024-01-09T10:36:24Z

Hello @serhii-chernenko
Thank you for your contribution.
Sorry, but now this PR has conflicts.
Could you please resolve it?
Thank you in advance.

hostep · 2024-01-09T13:56:36Z

@andrewbess : can we have a conclusion first here before more work is put into this? Should we downgrade less.js back to v3, or will we continue with fixing the less files themselves, so they are compatible both with v3 and v4?

andrewbess · 2024-01-12T18:45:24Z

@andrewbess : can we have a conclusion first here before more work is put into this? Should we downgrade less.js back to v3, or will we continue with fixing the less files themselves, so they are compatible both with v3 and v4?

Hello @hostep
Thank you for your question
I would prefer your fix from this PR instead of downgrading the version

glo71317 · 2024-01-17T13:13:42Z

Hi @andrewbess @hostep @serhii-chernenko @ihor-sviziev

Thanks to all for contributing and highlighting the issue. Surely we will take a look on this on priority.
Today we have the PO call. so we will discuss on this and will update here accordingly.

Thanks,

glo71317 · 2024-01-17T16:50:46Z

Hello Guys, We had discussed and agreed we will not downgrade v4 to v3. We will continue working with This PR
This is to be released in Magento 2.4.7

ihor-sviziev · 2024-01-18T08:09:54Z

So, based on #38109 (comment), I'm closing this pull request in favor to #38335.
@serhii-chernenko thank you so much for your contribution!

Update package.json.sample

e076804

serhii-chernenko mentioned this pull request Oct 20, 2023

Update lessjs to v3.13.1 #31618

Merged

5 tasks

serhii-chernenko referenced this pull request Oct 20, 2023

AC-8074: npm package grunt-contrib-less upgrade to new major version(…

203f415

…3.0.0)

m2-community-project bot added this to Pending Review in Pull Requests Dashboard Oct 20, 2023

m2-community-project bot added the Progress: pending review label Oct 20, 2023

Update package.json.sample

7dcac26

grunt-contrib-less and less packages downgraded to make source maps working for magento 2.4.7+

ihor-sviziev added Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Risk: low bugfix labels Oct 25, 2023

ihor-sviziev self-assigned this Oct 25, 2023

m2-community-project bot moved this from Pending Review to Review in Progress in Pull Requests Dashboard Oct 25, 2023

m2-community-project bot removed the Progress: pending review label Oct 25, 2023

ihor-sviziev added Progress: needs update Progress: pending review labels Oct 25, 2023

m2-community-project bot added the Progress: review label Oct 25, 2023

m2-community-project bot moved this from Review in Progress to Changes Requested in Pull Requests Dashboard Oct 25, 2023

ihor-sviziev moved this from Pending Review to Changes Requested in Pull Requests Dashboard Oct 25, 2023

m2-community-project bot removed Progress: pending review Progress: review labels Oct 25, 2023

ihor-sviziev approved these changes Oct 27, 2023

View reviewed changes

m2-community-project bot moved this from Changes Requested to Ready for Testing in Pull Requests Dashboard Oct 27, 2023

m2-community-project bot added Progress: ready for testing and removed Progress: needs update labels Oct 27, 2023

serhii-chernenko changed the title ~~Fix grunt compiled less sourcemaps~~ Downgrade the Node.js LESS compiler from v4 to v3 to avoid less compilation problems Oct 27, 2023

hostep approved these changes Oct 27, 2023

View reviewed changes

m2-community-project bot assigned hostep Oct 27, 2023

engcom-Delta added the Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it label Dec 6, 2023

hostep mentioned this pull request Jan 7, 2024

Put division calculations in less files in parentheses if they were not there alr… #38335

Merged

6 tasks

m2-assistant bot mentioned this pull request Jan 12, 2024

[Issue] Put division calculations in less files in parentheses if they were not there alr… #38351

Closed

5 tasks

ihor-sviziev closed this Jan 18, 2024

m2-community-project bot removed the Progress: ready for testing label Jan 18, 2024

m2-community-project bot removed this from Ready for Testing in Pull Requests Dashboard Jan 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Downgrade the Node.js LESS compiler from v4 to v3 to avoid less compilation problems #38109

Downgrade the Node.js LESS compiler from v4 to v3 to avoid less compilation problems #38109

serhii-chernenko commented Oct 20, 2023 •

edited

m2-assistant bot commented Oct 20, 2023

ihor-sviziev commented Oct 20, 2023

serhii-chernenko commented Oct 20, 2023

hostep commented Oct 21, 2023 •

edited

hostep commented Oct 21, 2023 •

edited

glo17680 commented Oct 23, 2023 •

edited

hostep commented Oct 23, 2023 •

edited

ihor-sviziev commented Oct 23, 2023

serhii-chernenko commented Oct 24, 2023

hostep commented Oct 24, 2023 •

edited

serhii-chernenko commented Oct 24, 2023

hostep commented Oct 24, 2023

serhii-chernenko commented Oct 24, 2023

ihor-sviziev commented Oct 25, 2023

ihor-sviziev commented Oct 25, 2023

magento-automated-testing bot commented Oct 25, 2023

serhii-chernenko commented Oct 27, 2023

hostep commented Oct 27, 2023 •

edited

serhii-chernenko commented Oct 27, 2023

glo17680 commented Dec 5, 2023 •

edited

hostep commented Jan 3, 2024 •

edited

hostep commented Jan 7, 2024

andrewbess commented Jan 9, 2024

hostep commented Jan 9, 2024

andrewbess commented Jan 12, 2024

glo71317 commented Jan 17, 2024

glo71317 commented Jan 17, 2024

ihor-sviziev commented Jan 18, 2024

Downgrade the Node.js LESS compiler from v4 to v3 to avoid less compilation problems #38109

Downgrade the Node.js LESS compiler from v4 to v3 to avoid less compilation problems #38109

Conversation

serhii-chernenko commented Oct 20, 2023 • edited

Description (*)

Related Pull Requests

Manual testing scenarios (*)

Contribution checklist (*)

m2-assistant bot commented Oct 20, 2023

ihor-sviziev commented Oct 20, 2023

serhii-chernenko commented Oct 20, 2023

hostep commented Oct 21, 2023 • edited

hostep commented Oct 21, 2023 • edited

glo17680 commented Oct 23, 2023 • edited

hostep commented Oct 23, 2023 • edited

ihor-sviziev commented Oct 23, 2023

serhii-chernenko commented Oct 24, 2023

hostep commented Oct 24, 2023 • edited

serhii-chernenko commented Oct 24, 2023

hostep commented Oct 24, 2023

serhii-chernenko commented Oct 24, 2023

ihor-sviziev commented Oct 25, 2023

ihor-sviziev commented Oct 25, 2023

magento-automated-testing bot commented Oct 25, 2023

serhii-chernenko commented Oct 27, 2023

hostep commented Oct 27, 2023 • edited

serhii-chernenko commented Oct 27, 2023

glo17680 commented Dec 5, 2023 • edited

hostep commented Jan 3, 2024 • edited

hostep commented Jan 7, 2024

andrewbess commented Jan 9, 2024

hostep commented Jan 9, 2024

andrewbess commented Jan 12, 2024

glo71317 commented Jan 17, 2024

glo71317 commented Jan 17, 2024

ihor-sviziev commented Jan 18, 2024

serhii-chernenko commented Oct 20, 2023 •

edited

hostep commented Oct 21, 2023 •

edited

hostep commented Oct 21, 2023 •

edited

glo17680 commented Oct 23, 2023 •

edited

hostep commented Oct 23, 2023 •

edited

hostep commented Oct 24, 2023 •

edited

hostep commented Oct 27, 2023 •

edited

glo17680 commented Dec 5, 2023 •

edited

hostep commented Jan 3, 2024 •

edited