Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update lessjs to v3.13.1 #31618

Merged
merged 1 commit into from
Aug 6, 2021
Merged

Update lessjs to v3.13.1 #31618

merged 1 commit into from
Aug 6, 2021

Conversation

mrtuvn
Copy link
Contributor

@mrtuvn mrtuvn commented Jan 12, 2021 •
edited by ihor-sviziev
Loading

Description (*)

Update old version to recently newer
2.3.1 to 3.13.1
Current magento used lessjs v2.3.1.
Official version is https://cdn.jsdelivr.net/npm/[email protected]
https://www.jsdelivr.com/package/npm/less
For maintain compatible we can stay at v3 for a while!. Current official less released v4.1.0

Official less website http://lesscss.org/

This file allow to browser compile less file to css on-flies. Allow developer can check styles from browser without need run deploy ! Note this feature not work in production mode

Idealy lessjs the version should be same as lessphp compile version

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Node should have version >=12 <14
Current grunt not work properly in node v14+

Test case 1

  1. Enable developer mode
  2. Enable Client side compilation in Advanced Configuration
    (Stores > Configuration > Advanced > Developer
    Frontend Development Workflow => Workflow type : Client side less compilation)
  3. Save and clear cache config admin
  4. Go to frontend and verify page render properly without break or glitch

Test case 2

  1. Change package.json.sample => package.json
  2. Install package by run command
    npm install
  3. from command-line run
    node_modules/.bin/lessc --version and check properly version less compare with (package.json)

Test case 3
Run grunt workflow without problem
https://devdocs.magento.com/guides/v2.4/frontend-dev-guide/css-topics/css_debug.html

https://devdocs.magento.com/guides/v2.4/test/js/jasmine.html
Noted before run grunt spec must deploy static contents first

Questions or comments

CC: @Leland @krzksz @ptylek what do you guys think on this ?

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)
  • All automated tests passed successfully (all builds are green)

Resolved issues:

  1. resolves [Issue] Update lessjs to v3.13.1 #31824: Update lessjs to v3.13.1

@m2-assistant
Copy link

m2-assistant bot commented Jan 12, 2021

Hi @mrtuvn. Thank you for your contribution
Here is some useful tips 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:

  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE,
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

@mrtuvn mrtuvn mentioned this pull request Jan 12, 2021
Closed
4 tasks
@ihor-sviziev ihor-sviziev added Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests improvement Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. labels Jan 12, 2021
@ihor-sviziev ihor-sviziev self-assigned this Jan 12, 2021
@ihor-sviziev
Copy link
Contributor

@magento run all tests

@hostep
Copy link
Contributor

hostep commented Jan 12, 2021

Maybe best to double check what version the less.php library supports as well.
This seems to be defined over here, so version 2.5.3 at the time of writing, but I have no idea how accurate it is as it was updated almost 4 years ago: wikimedia/less.php@712473a

And this also needs to be aligned with the less version which is installed through npm, which at the moment seems to be:

$ git pull upstream 2.4-develop
$ rm -R node_modules
$ npm install

$ node_modules/.bin/lessc --version
lessc 2.7.3 (Less Compiler) [JavaScript]

=> version 2.7.3

But Magento developers can easily override this version should they want to, so all kinds of versions could potentially be used in the wild ...

Ideally all 3 less compilation libraries included in Magento should be using the same version (or at least versions as compatible as possible with each other). But I have no idea how realistic this is.

Thoughts?

@gabrieldagama gabrieldagama added this to the 2.5 milestone Jan 12, 2021
@gabrieldagama gabrieldagama added Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Severity: S2 Major restrictions or short-term circumventions are required until a fix is available. labels Jan 12, 2021
@gabrieldagama gabrieldagama removed the Severity: S2 Major restrictions or short-term circumventions are required until a fix is available. label Jan 12, 2021
@ihor-sviziev
Copy link
Contributor

@mrtuvn @hostep
Could you check the results across different LESS versions for Blank and Luma themes?
If the result is the same on these versions - I think we're fine with an update also less version used during npm install. If not - let's not upgrade. What do you think?

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@andrewbess
Copy link

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

andrewbess
andrewbess approved these changes Aug 5, 2021
View reviewed changes
Copy link

@andrewbess andrewbess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved
Waiting for finishing tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

9 similar comments
@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@sivaschenko
Copy link
Member

Magento\BundleImportExport\Model\BundleStagingTest::testImportExport is failing on platform-health

@sivaschenko sivaschenko merged commit d0120fd into magento:platform-health Aug 6, 2021
@m2-assistant
Copy link

m2-assistant bot commented Aug 6, 2021

Hi @mrtuvn, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@andrewbess andrewbess deleted the update-lessjs-v3 branch August 6, 2021 11:24
@engcom-Lima engcom-Lima mentioned this pull request Oct 8, 2021
Closed
5 tasks
This was referenced Oct 20, 2023
Merged
Closed
@serhii-chernenko
Copy link

serhii-chernenko commented Oct 20, 2023 •
edited
Loading

I don't even believe that no one didn't comment this PR 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:
image

grunt-contrib-less ~2.1.0:
image

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

steps to reproduce:

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

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 the 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.
image

PR with fix created:
#38109

serhii-chernenko referenced this pull request Oct 20, 2023
@glo17680
AC-8074: npm package grunt-contrib-less upgrade to new major version(…
203f415 
…3.0.0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@engcom-Hotel engcom-Hotel engcom-Hotel left review comments

@eduard13 eduard13 eduard13 requested changes

@ihor-sviziev ihor-sviziev ihor-sviziev requested changes

@andrewbess andrewbess andrewbess approved these changes

Assignees

@ihor-sviziev ihor-sviziev

@andrewbess andrewbess

@eduard13 eduard13

Labels
Area: Lib/Frontend Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests improvement Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: ready for testing Project: Platform Health Release Line: 2.5 Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Investigate upgrade/create POC to the latest LessCSS [Issue] Update lessjs to v3.13.1
10 participants
@mrtuvn @ihor-sviziev @hostep @gabrieldagama @magento-engcom-team @engcom-Hotel @sivaschenko @andrewbess @serhii-chernenko @eduard13