connorskees / grass

A Sass compiler written purely in Rust
https://docs.rs/grass/
MIT License
497 stars 38 forks source link

`map.get` doesn't support `$keys...` argument #80

Closed pr9000 closed 1 year ago

pr9000 commented 1 year ago

Dear Grass Team,

I hope this message finds you well. I wanted to bring to your attention an issue I encountered while using the Grass library to compile the Fontsource mixins.scss file. The issue pertains to the map.get function and its behavior when attempting to retrieve a nested value from a map.

Description: When using the map.get function in Grass, I noticed that it works perfectly fine when retrieving a value from a top-level map. However, when trying to retrieve a value from a nested map using the same function, an error is thrown stating that only two arguments are allowed instead of three.

I encountered this issue specifically when compiling the Fontsource mixins.scss file, which relies on the map.get function to retrieve nested values from the metadata map.

For example, consider the following code snippet from mixins.scss: ...

$display: if($display, $display, swap); $formats: if(not $formats or $formats == all, if($isVariable, woff2, (woff2, woff)), $formats); $subsets: if( $subsets, if($subsets == all, map.get($metadata, subsets), $subsets), map.get($metadata, defaults, subset) ); $weights: if( $weights, if($weights == all, map.get($metadata, weights), $weights), map.get($metadata, defaults, weight) ); $styles: if( $styles, if($styles == all, map.get($metadata, styles), $styles), map.get($metadata, defaults, style) ); $axes: if( $axes, if($axes == all, full, $axes), if($isVariable, if(map.has-key($metadata, axes, wght), wght, full), null) );

...

In this case, the map.get function fails when attempting to retrieve the nested values from the $metadata map within the if statements.

Error Message: Error: Only 2 arguments allowed, but 3 were passed. ╷ 48 │ map.get($metadata, defaults, subset) │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ╵ ./mixins.scss:48:5

Test to Replicate the Issue: To replicate the issue, I have created a simple test case using Grass and Sass that demonstrates the problem with the map.get function. Here's the code snippet:

//nestedmap.scss @use 'sass:map'; $colors: ( primary: ( text: #FF0000, bg: #990000, ), secondary: ( text: #00FF00, bg: #009900, ), ); .text-primary { color: map.get($colors, primary); } .bg-primary { background-color: map.get($colors, primary, bg); // Error occurs here } .text-secondary { color: map.get($colors, secondary); } .bg-secondary { background-color: map.get($colors, secondary, bg); // Error occurs here }

grass nestedmap.scss nestedmap.css Error: Only 2 arguments allowed, but 3 were passed. ╷ 19 │ background-color: map.get($colors, primary, bg); // Error occurs here │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ╵ ./nestedmap.scss:19:21

When attempting to compile the above Sass code using the Grass compiler, the same error occurs, indicating that only two arguments are allowed instead of three when using the map.get function to retrieve nested values.

Expected Behavior: Ideally, the map.get function in Grass should support retrieving nested values from a map without any errors.

I hope this information helps in addressing the issue. If you need any further details or assistance, please let me know. Thank you for your attention to this matter, and I appreciate your efforts in maintaining the Grass library.

Best regards

connorskees commented 1 year ago

I'll try to implement this functionality sometime this week. Last time I took a look at this, there were some weird differences between the function in the global scope and the one in the map module.

pr9000 commented 1 year ago

Thanks. I will happy to test it for you. Just let me know.

greenwoodcm commented 1 year ago

i came across this the other day as well. i'd happily take a stab at a PR, although i'm unfamiliar with the differences you mention between map-get and map.get

greenwoodcm commented 1 year ago

i noticed it when building the test site content for the following PR:

https://github.com/getzola/zola/pull/2242/files

connorskees commented 1 year ago

The difference is related to error messages, but I believe it is unrelated to the actual implementation of the functions. If you were to implement this, you can just ignore that.

I'd be happy to accept a PR for this.

The code for implementing this lives here: https://github.com/connorskees/grass/blob/master/crates/compiler/src/builtin/functions/map.rs#L3

For reference, the dart-sass implementation is here: https://github.com/sass/dart-sass/blob/main/lib/src/functions/map.dart#L38

Some of the code for implementing builtin functions isn't the cleanest. I think you'd want do something like let keys = args.get_variadic() (example here https://github.com/connorskees/grass/blob/master/crates/compiler/src/builtin/functions/list.rs#L239) and iterate through the keys.

greenwoodcm commented 1 year ago

thanks for the background, i'll take a stab at it

connorskees commented 1 year ago

83 has been released to crates.io as part of 0.13.0. Thanks again for raising this issue, and thanks to @greenwoodcm for implementing a fix!