oddbird / accoutrement

Combined Sass Accoutrement tools, with option for individual module imports
MIT License
37 stars 6 forks source link

register-function fails when using sass modules (@use) #46

Closed johncrim closed 4 years ago

johncrim commented 4 years ago

Most of the accoutrement functionality has worked well for me using sass modules, though I realize it's not sass module native. However, register-function does not work with sass modules (unless I'm missing something).

This code:

@use 'accoutrement/sass' as a;
@use 'sass:color';
@use 'sass:map';
@use 'sass:meta';

@function add-transparency($color, $ignored) {
  @return color.change($color, $alpha: 0.8);
}
@include a.register-function('add-transparency');

$colors: (
  'bg': purple ('add-transparency': 'ignored')
);
@include a.add-colors($colors);

@debug a.color('bg');

Fails with error:

$ sass reg-func-use.scss -I ./node_modules/
Error: "[register-function] add-transparency() is not available to register with Accoutrement"
    ╷
159 │     @return _a_error($error, $origin);
    │             ^^^^^^^^^^^^^^^^^^^^^^^^^
    ╵
  node_modules\accoutrement\sass\core\_register.scss 159:13  -a-registration()
  node_modules\accoutrement\sass\core\_register.scss 84:14   register-function()
  reg-func-use.scss 9:1                                      root stylesheet
error Command failed with exit code 65.

A version of the same code rewritten to use @import works as expected:

@import 'accoutrement/sass';

@function add-transparency($color, $ignored) {
  @return change-color($color, $alpha: 0.8);
}
@include register-function('add-transparency');

$colors: (
  'bg': purple ('add-transparency': 'ignored')
);
@include add-colors($colors);

@debug color('bg');

In case anyone wants a workaround, the following code is working for me, as a replacement for register-function:

@use 'accoutrement/sass' as a;
@use 'sass:map';
@use 'sass:meta';

a.$functions: map.merge(a.$functions, ('add-transparency': meta.get-function('add-transparency')));
johncrim commented 4 years ago

Here's a Gist to make it a little easier to run this repro, and the working @import version: https://gist.github.com/johncrim/64e2ac1a781219ae38843588e9edaa7d

mirisuzanne commented 4 years ago

I think this should work if you pass the captured function rather than the string function-name:

@include a.register-function(meta.get-function('add-transparency'));

The goal of register-function() is not to handle function-capturing for you – that's something that will have to happen in local scope with Sass modules – but to register that captured function with Accoutrement, and make it available to map processing under any alias you want.

It was designed to accept strings for older Sass versions, and capture global functions in intermediate versions, but it can also accept first-class functions if you use Sass modules. There's not really any way for us to handle that string-to-function step in the module syntax, since your local functions are not part of a global scope that Accoutrement can access.

We could possibly provide built-in access to Sass core functions, but that would be a new feature…

mirisuzanne commented 4 years ago

On closer inspection, there does seem to be a bug a bit later in the process. Checking if function-exists will always fail…

mirisuzanne commented 4 years ago

@johncrim Try accoutrement@2.1.3-beta.1 with a locally-captured function and see if that solves the problem for you?

johncrim commented 4 years ago

I had tried manually capturing the function (and it didn't work, with the same error).

Will try the new version...

johncrim commented 4 years ago

I've added accoutrement@2.1.3-beta.1 and am still seeing an error, using manual function capture:

Changing to:

@include a.register-function(meta.get-function('add-transparency'));

Results in this error:

$ sass reg-func-use.scss -I ./node_modules/
Error: get-function("add-transparency") isn't a valid CSS value.
    ╷
190 │     $function-name: str-slice(unquote('#{$function}'), 14, -2);
    │                                          ^^^^^^^^^
    ╵
  node_modules\accoutrement\sass\core\_register.scss 190:42  -a-registration()
  node_modules\accoutrement\sass\core\_register.scss 100:14  register-function()
  reg-func-use.scss 11:1                                     root stylesheet
error Command failed with exit code 65.

It looks like it's trying to re-capture the function by string name.

Thank you for looking into this, and for making this very useful project open-source.

mirisuzanne commented 4 years ago

I'll just cut that line: it's actually trying to extract the function name from the first-class function you passed in. It's a bit of a hack, and I knew it was a long-shot. Just means you'll need to provide both the function and a name:

@include a.register-function(meta.get-function('add-transparency'), 'add-transparency');

This should be fixed and released in 2.1.3 now. Thanks for helping debug!

johncrim commented 4 years ago

Confirming that this works - thank you!

Also, the new error message is nice when the dev forgets to include the function name:

$ sass reg-func-use.scss -I ./node_modules/
Error: "[register-function] Provide at least one name for this function registration"
    ╷
202 │     @return _a_error($error, $origin);
    │             ^^^^^^^^^^^^^^^^^^^^^^^^^
    ╵
  node_modules\accoutrement\sass\core\_register.scss 202:13  -a-registration()
  node_modules\accoutrement\sass\core\_register.scss 103:14  register-function()
  reg-func-use.scss 11:1                                     root stylesheet