monvillalon / sass-values-loader

Webpack loader to expose sass variables to javascript
3 stars 11 forks source link

Breaks with kebab-case variable names #1

Open jerryjappinen opened 6 years ago

jerryjappinen commented 6 years ago

Love the project, I'm desperately looking for a reliable implementation of this functionality for webpack. Ran into an issue as soon as I tried it though, namely it seems that the project doesn't escape or transform any of the variable names and breaks if kebab-case is used (which is quite common with CSS):

constants.scss:

$color-transparent: transparent;
$color-black: #000;
$color-white: #fff;

Webpack error:

 ERROR  Failed to compile with 1 errors                                                                                                                             12:56:40

 error  in ./src/styles/constants.scss

Module parse failed: Unexpected token (1:16)
You may need an appropriate loader to handle this file type.
| export var color-transparent = "rgba(0, 0, 0, 0)"
| export var color-black = "rgb(0, 0, 0)"
| export var color-white = "rgb(255, 255, 255)"

 @ ./src/config/config.styles.js 5:0-63
 @ ./src/config/dev/index.js
 @ ./src/plugins/router.js
 @ ./src/vendor/vue.js
 @ ./src/main.js
 @ multi (webpack)-dev-server/client?http://localhost:8080 webpack/hot/dev-server ./src/main.js

Other plugins I've used transform the keys to camelCase (with an option to disabled this). Adding an option to decide what the casing is should be relatively easy, and lodash has good support for different transformations.

Didn't have time to try to fix it or file a PR yet, but decided to still file this issue.

monvillalon commented 6 years ago

Sorry for the delay on this and thanks for reporting.

My approach on solving this is that any non valid javascript identifiers will be skipped unless an option is set determining how to resolve the error. The option would receive either snake_case, camelCase or PascalCase.

So that the variable $color-black would be exported as color_black if for example snake_case is set.

Does this work for your use case?

Option Values Default Description
normalization none,pascal,snake,camel none Transform the variable identifiers based on a normalization strategy. If set to none all variables will be exported as is, excluding any names that are not valid indentifers.
jerryjappinen commented 6 years ago

I solved this in my fork by just converting everything to camelCase for the exports, and converting the keys to camelCase as well unless the preserveKeys option is turned on.

I think for most users it's fine to convert the export names to camelCase to ensure builds work consistently, but yeah of course adding the normalisation strategy as an option is a nice addition.

monvillalon commented 6 years ago

@Eiskis that is the approach I'm taking but there are some differences.

  1. You can specify your normalization method (snake, underscore, pascal and camel).
  2. Invalid identifiers are also checked like reserved words (ex: bool, 123_I_start_with_numbers) are checked and prefixed with underscore (bool to _bool)
  3. You can uppercase the exports (in case you really want to use reserved words)

Currently writing tests

wataru358 commented 6 years ago

@Eiskis @monvillalon thank both of you. I used Eiskis folk for our internal project, but sass vars in js idea was postponed for a time being. But I will be using it for my personal project. Eiskis approach was suffice, but adding more option seems a good idea.

jerryjappinen commented 6 years ago

@monvillalon nice to see you're picking up the development again!

jjenzz commented 6 years ago

@Eiskis I am using your fork of this project but cannot get it working. I have updated the Webpack postLoaders as follows:

{
  test: /variables\/.+\.scss$/,
  loader: "sass-vars-to-js-loader"
}

And get the following error (looks like an scss-parser error):

Module build failed: Error: Expecting punctuation: "}" (21:1)
    at InputStream.err ([..]/node_modules/scss-parser/dist/input-stream.js:122:13)
    at Object.err ([..]/node_modules/scss-parser/dist/input-stream.js:161:20)
    at TokenStream.err ([..]/node_modules/scss-parser/dist/token-stream.js:297:40)
    at Object.err ([..]/node_modules/scss-parser/dist/token-stream.js:634:20)
    at Parser.skip_type ([..]/node_modules/scss-parser/dist/parse.js:199:21)
    at Parser.skip_punctuation ([..]/node_modules/scss-parser/dist/parse.js:214:29)
    at Parser.parse_block ([..]/node_modules/scss-parser/dist/parse.js:547:37)
    at Parser.parse_rule ([..]/node_modules/scss-parser/dist/parse.js:692:24)
    at Parser.parse_node ([..]/node_modules/scss-parser/dist/parse.js:357:51)
    at Parser.parse_block ([..]/node_modules/scss-parser/dist/parse.js:539:25)
    at Parser.parse_rule ([..]/node_modules/scss-parser/dist/parse.js:692:24)
    at Parser.parse_node ([..]/node_modules/scss-parser/dist/parse.js:357:51)
    at Parser.parse_stylesheet ([..]/node_modules/scss-parser/dist/parse.js:267:25)
    at module.exports ([..]/node_modules/scss-parser/dist/parse.js:748:17)
    at Object.parse ([..]/node_modules/scss-parser/dist/index.js:27:10)
    at transformSASSFile ([..]/node_modules/sass-vars-to-js-loader/src/extract.js:29:25)

My Sass map is as follows:

$palette: (
  purple-400: #bb5fdd,
  purple-200: #575778,
  blue-600: #404088,
  blue-400: #0c5690,
  blue-200: #30a0ff
  // etc.
);

Do you have any idea what I am doing wrong here?

jerryjappinen commented 6 years ago

@jjenzz First thing that comes to mind: does it work if you add a trailing comma in $palette?

Sometimes I really don't like Sass.

rhys-vdw commented 6 years ago

@Eiskis can you either abandon your fork in preference of this project, or enable issue reporting on your fork? I tried our your branch which is more frequently updated, but I can't report bugs so I don't think it's a good choice for us moving forward.

(Sorry @monvillalon 😉)

jerryjappinen commented 6 years ago

@rhys-vdw issues enabled!