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.

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

[5.2] Build process: Change Dart-Sass to new API #44659

Open
wants to merge 3 commits into
base: 5.2-dev
Choose a base branch
from

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented Dec 22, 2024

Summary of Changes

The Dart-Sass engine has a "new" API, where instead of renderSync and a configuration object, it just uses compile() with the filename instead. As far as I can see, this PR fixes that deprecation warning. Since this only affects our build process, I would consider this to be safe for adoption in 5.2.

Testing Instructions

Codereview.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@richard67
Copy link
Member

richard67 commented Jan 18, 2025

I have tested this item ✅ successfully on 37b0dfa

I think code review is not really sufficient for testing this PR, so I have done (in addition to the review) following tests:

  1. Run a php ./build/build.php --remote=HEAD --exclude-gzip --exclude-zstd on a clean branch one time without and one time with this PR applied and compare the content of the created packages build/tmp/packages/Joomla_5.2.4-dev-Development-Full_Package.zip.
    Hint: You can fetch this PR into a local branch "test-pr-44659" for the test with
    git fetch upstream pull/44659/head:test-pr-44659
    and then checkout that branch with
    git co test-pr-44659
    Result: There are no differences except of the usual different autoloader IDs and partly different ordering of assets in joomla.asset.json files.

  2. Run npm ci one time without and one time with this PR applied under the same starting conditions (clean branch with composer installhaving run) and compare the output.
    Hint: For easy comparison of the output you can run npm and redirect all standard and error output into a test file on Linux or Windows with WSL like this:
    npm ci > ./build/npm-ci-with-pr.txt 2>&1
    Result: The deprecation messages Deprecation [legacy-js-api]: The legacy JS API is deprecated and will be removed in Dart Sass 2.0.0. have disappeared with this PR applied.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44659.

@richard67 richard67 added the Maintainers Checked Used if the PR is conceptional useful label Jan 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintainers Checked Used if the PR is conceptional useful PR-5.2-dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants