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

Allow to replace tilde sign with given path value to fix the tilde sign #585

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

ghanu119
Copy link

@ghanu119 ghanu119 commented Jul 2, 2022

Allow to replace tilde sign with given path value to fix the tilde sign path resolution

Now Tilde( ~ ) can be replaced with the varaibles path_for_tilde.

When there is ~ sign exists in import line then define the replacement like:
$compiler->addVariables(['path_for_tilde' => 'PATH_TO_REPLACE'])

AS PER NEW CHANGES
With working JS frameworks sometimes some library path starts with the ~ [Tilde] sign and this variable will allow you to set the starting ~ to replace the directory path.

$compiler->addVariables([
    'path_for_tilde' => [ 'PATH_TO_REPLACE' ]
]);

@stof
Copy link
Member

stof commented Jul 4, 2022

Replacing that in all strings does not make any sense and would break spec compliance.

We already have a hook for import paths, where you can pass a callable that performs a custom resolution.

@ghanu119
Copy link
Author

ghanu119 commented Jul 4, 2022

ImportPath hook is not working when there is a import with ~ sign. As sass-loader mange to replace the ~ sign with the node_modules where this package can't work with ~ import sign and also not looked into the the path that hooked using the importPaths.
For exmple,
@import '~bootstrap/scss/bootstrap.scss'; @import '~bootstrap-select/sass/bootstrap-select.scss';
Now importPath hook not work with this type of imports.

@stof
Copy link
Member

stof commented Jul 4, 2022

What do you mean by "does not work" ?

Your proposal here impacts all strings, not only imports, so it is clearly not something we will merge.

@stof stof added the Waiting answer The issue waits for an answer of the reporter label Jul 26, 2022
@ghanu119
Copy link
Author

@stof
New changes made and fixed as per your suggestion.

test.scss

@import '~bootstrap/scss/bootstrap.scss';
@import '~test/custom~/demo.scss'; // It will only set the path for the first tilde and not effect on second tilde.

.my-custom {
    .sub-custom {
        font-size: 12px;
        font-weight: bold;
    }
}

PHP CODE

<?php

require_once 'scss.inc.php';

use ScssPhp\ScssPhp\Compiler;
use ScssPhp\ScssPhp\OutputStyle;

$compiler = new Compiler();
$nodeModule = $_SERVER['DOCUMENT_ROOT']  . '\scsstest\node_modules\\';
$compiler->setOutputStyle( OutputStyle::COMPRESSED );
$compiler->addImportPath( $nodeModule );
$compiler->addVariables([
    'path_for_tilde' => [$nodeModule]
]);

echo $compiler->compileString(file_get_contents($_SERVER['DOCUMENT_ROOT']  . '..\scsstest\test.scss' ))->getCss();

@ghanu119
Copy link
Author

ghanu119 commented Dec 6, 2023

@stof
You have to also merge this PR as it will improve the functionality and able to read files from the node_modules folder and there are lots of projects which use the plugin and there css used inside the project.
Please check this.

@stof
Copy link
Member

stof commented Dec 6, 2023

This behavior is not part of the Sass standard. dart-sass does not support those ~ prefix either. So it does not fit the scope of the project.
Note that this ~ convention is something that is implemented only by the sass-loader of webpack. And the Sass team explicitly rejected bringing this convention to the core (they just released a new feature that will allow webpack to provide an alternative syntax supported by the Sass team).

As Scssphp 2.0 will not have non-standard features, it would mean I would have to mark that feature as deprecated as soon as it is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Waiting answer The issue waits for an answer of the reporter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants