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

Add support for named converted output files #30

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

Conversation

evertharmeling
Copy link
Contributor

@evertharmeling evertharmeling commented Nov 9, 2023

As I have the following setup in my application:

assets/
  admin/
    ...
    scss/
      app.scss
  website/
    ...
    scss/
      app.scss

My current symfonycasts_sass config:

symfonycasts_sass:
    root_sass:
        - '%kernel.project_dir%/assets/admin/scss/app.scss'
        - '%kernel.project_dir%/assets/website/scss/app.scss'

Would result in 1 file; var/sass/app.output.css, as all root_sass paths with the same filename will be stored under the same css filename, thus overwriting the file.

With this PR support is added to be able to influence the name of the converted css-file and still be BC with the current config / output.

The following symfonycasts_sass config:

symfonycasts_sass:
    root_sass:
        admin: '%kernel.project_dir%/assets/admin/scss/app.scss'
        website: '%kernel.project_dir%/assets/website/scss/app.scss'

Would result in 2 files

  • var/sass/admin.output.css
  • var/sass/website.output.css

The only caveat is, that the following config could result in an unexpected output:

symfonycasts_sass:
    root_sass:
        20: '%kernel.project_dir%/assets/admin/scss/app.scss'
        website: '%kernel.project_dir%/assets/website/scss/app.scss'

Would result in 2 files

  • var/sass/app.output.css (and not var/sass/20.output.css)
  • var/sass/website.output.css

As I'm currently 'ignoring' numbered keys as the array of paths gets numbered keys automatically if specific keys aren't used, to be BC.

Todo

  • Add support configuring custom filename per sass-path
  • Add support rendering the right (custom filename) sass-path
  • Add tests
  • Docs

@evertharmeling

This comment was marked as outdated.

@evertharmeling

This comment was marked as outdated.

src/AssetMapper/SassCssCompiler.php Outdated Show resolved Hide resolved
src/SassBuilder.php Outdated Show resolved Hide resolved
@evertharmeling
Copy link
Contributor Author

Ofcourse, good call! Fixed it!

@evertharmeling

This comment was marked as off-topic.

@evertharmeling

This comment was marked as outdated.

@bocharsky-bw
Copy link
Member

... Although throwing an exception to have unique scss-filenames would be desired in this config, to not end up files being overwritten like stated in the issue. Thoughts?

Throwing an exception on file overwriting sounds to me like a good idea. Could you do that too? And in the exception message, we probably could mention that devs can leverage custom naming for the paths.

@evertharmeling evertharmeling marked this pull request as ready for review November 16, 2023 10:20
Copy link
Member

@bocharsky-bw bocharsky-bw left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I would like to hear more thoughts on this new feature as this might bring some BC break in the future if we decided we don't need this feature.

Also, it seems like it would be a nice idea to mention this new feature in the docs as it might be a little-known fact for users.

@evertharmeling
Copy link
Contributor Author

Sure, I get it! I will add it to the docs later on

@@ -61,7 +61,7 @@ public function getConfigTreeBuilder(): TreeBuilder
->end()
->validate()
->ifTrue(static function (array $paths): bool {
if (1 === \count($paths)) {
if (1 === \count($paths) || !array_is_list($paths)) {
Copy link

Choose a reason for hiding this comment

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

If you want to allow using maps and not just lists, you need to use ->useAttributeAsKey. Otherwise, merging multiple config files (note that for this matter, a when@prod section counts as a separate config for the merging process) together won't preserve keys (and devs using a XML config file would not be able to specify a key)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how I would define that?

$rootNode
    ->children()
        ->arrayNode('root_sass')
            ->info('Path to your Sass root file')
            ->cannotBeEmpty()
            ->useAttributeAsKey('???')  // <--- here?
            ->scalarPrototype()
                ->end()
            ->validate()
                ...

With or without ->useAttributeAsKey(), and manually testing it with an extra when@dev config for example, does not change the array passed to the validate-callback function? And what would be the $name variable to pass to ->useAttributeAsKey()?

Can you point me to an example definition?

Copy link

Choose a reason for hiding this comment

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

@evertharmeling you would have to define a when@dev section (or a separate file) that also configures this root_sass node, so that some merging actually happens (if the value is defined only in 1 place, the merging is bypassed entirely, which will preserve keys even if the node is not configured for that).

For the name being passed, this would be the name of the attribute used in an XML config (where it cannot be an array key). In this case, output looks like a good option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Testing with:

config/packages/symfony_casts.yaml:

symfonycasts_sass:
    root_sass:
        website: 'assets/website/scss/app.scss'

config/packages/test.yaml:

symfonycasts_sass:
    root_sass:
        test: 'assets/website/scss/app.scss'
        website: 'assets/test/scss/app.scss'

when@dev:
    symfonycasts_sass:
        root_sass:
            test2: 'assets/website/scss/app.scss'

Running symfony console debug:config SymfonycastsSassBundle:

Current configuration for "SymfonycastsSassBundle"
==================================================

symfonycasts_sass:
    root_sass:
        website: assets/test/scss/app.scss
        test: assets/website/scss/app.scss
        test2: assets/website/scss/app.scss
    binary: null
    embed_sourcemap: true

Even replacing an entry with same key website seems to work?

I'm fine with adding ->useAttributeAsKey('output'), but can't reproduce it with these examples or am I still overlooking something?


foreach ($this->sassPaths as $identifier => $configuredSassPath) {
// as the configured paths include the project dir, we need to subtract it to be able to compare the paths
$logicalPath = str_replace($this->projectRootDir.'/assets/', '', $configuredSassPath);
Copy link

Choose a reason for hiding this comment

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

be careful about windows paths when doing str_replace.

And also be careful that you don't remove only a prefix. If the project root dir is /app, things will be if you have a path like /app/assets/sub/app/assets

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good one, I refactored it to make use of substr. How would I need to address any Windows related problems?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants