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

Breaks with kebab-case variable names #1

Open
jerryjappinen opened this issue Jan 4, 2018 · 9 comments
Open

Breaks with kebab-case variable names #1

jerryjappinen opened this issue Jan 4, 2018 · 9 comments

Comments

@jerryjappinen
Copy link

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

@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
Copy link

wataru358 commented Feb 23, 2018

@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
Copy link
Author

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

@jjenzz
Copy link

jjenzz commented Mar 27, 2018

@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
Copy link
Author

@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
Copy link

rhys-vdw commented Sep 17, 2018

@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
Copy link
Author

@rhys-vdw issues enabled!

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

No branches or pull requests

5 participants