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

Indented Sass #3

Closed
du5rte opened this issue Aug 4, 2015 · 8 comments
Closed

Indented Sass #3

du5rte opened this issue Aug 4, 2015 · 8 comments

Comments

@du5rte
Copy link

du5rte commented Aug 4, 2015

Hey, is there any change to make this support indented sass styles? Similar to how webpack's sass-loader or gulp-sass or use node-sass but with support for both version.

@du5rte
Copy link
Author

du5rte commented Aug 4, 2015

I'm not way a expert with nodeJS but I have worked a very elegant hack.
my fork

Seems like node-sass render.file some how knows when a file is .scss or .sass and but render.data does not.

The easiest way I found to fix this was by checking if data contains semi columns or opening curly braces. Note the added space in ' {', as curly braces are still for interpolation in sass #{$value}.
Not the best fix as there's still a loophole if .scss is uglified

if (input.data.indexOf(';') === -1 && input.data.indexOf(' {') === -1) {
  input.indentedSyntax = true;
}
// Passes as sass
=test($property, $value)
  #{$property}: $value

body
  +test(color, red)
// note if uglyfied it will pass as sass
@mixin yo($property, $value){#{$property}: $value}
body{@include yo(color, red)}

I also noticed @import would not work so I added this hack which includes the path from the module.parent.filename into includePaths

var path = require('path');
var context = path.dirname(module.parent.filename);
input.includePaths = [context];

I know you can pass in options in when using it javascript but im using it as a jade template filter which doesn't allow me doing it and adds versatility for other uses.

body
  style
    :sass
      @import awesomeMixinLibrary.scss
      h1
        color: &
        +span(6)
        +center(vertical)

I hope this helps anyone else

@tunnckoCore
Copy link
Member

Not good idea! But thanks for the opening this issue.

Is there some module that handle both cases? There should have module like that with some option to support that, and will just use him. Options is completely optional, JStransformer dont do anything, we just provide options to user.

I'll review the issue.

@RobLoach
Copy link
Member

RobLoach commented Aug 5, 2015

@Monteirocode To get indented syntax, you'd manually pass options.indentedSyntax = true.

Not sure if automatic detection of it is a good idea directly in jstransformer-scss. I think having easy use of it is essential though. Considering that, I've opened up a PR over at jstransformer-sass to force indentedSyntax though: jstransformers/jstransformer-sass#4

Might be worth a shot at using that one! Provide your thoughts over there, Forbes also had some thoughts on it.

@tunnckoCore
Copy link
Member

yep.. it works partially, only test for renderFile fails. You dont need to detect, it will just works for both (i think) if indentedSyntax: true (damn, i <3 good coders!)

Error: invalid top-level expression
           at Object.module.exports.renderSync (/home/charlike/dev/jstransformer-scss/node_modules/node-sass/lib/index.js:409:22)
           at Object.exports.renderFile (/home/charlike/dev/jstransformer-scss/index.js:42:18)
           at TestCase.fn (/home/charlike/dev/jstransformer-scss/node_modules/test-jstransformer/index.js:148:32)
           at /home/charlike/dev/jstransformer-scss/node_modules/testit/lib/suite.js:74:29
           at /home/charlike/dev/jstransformer-scss/node_modules/testit/node_modules/promise/lib/es6-extensions.js:18:19
           at flush (/home/charlike/dev/jstransformer-scss/node_modules/testit/node_modules/asap/asap.js:27:13)
           at doNTCallback0 (node.js:408:9)
           at process._tickCallback (node.js:337:13)

@tunnckoCore
Copy link
Member

Okey, it was the file extension, nah.

But okey, @Monteirocode it's not our job to do detecting and do some tricks and hacks to allow to income both syntaxes. It's works for both syntaxes, you just should add indentedSyntax

@tunnckoCore
Copy link
Member

@Monteirocode just use jstransformer-sass, or create some package with that hack, or open PR to node-sass to suggest and discuss the problem.

@RobLoach
Copy link
Member

RobLoach commented Aug 5, 2015

use jstransformer-sass, or create some package with that hack, or open PR to node-sass to suggest and discuss the problem.

I'm suggesting we adapt jstransformer-sass: jstransformers/jstransformer-sass#4

@tunnckoCore
Copy link
Member

Yea, i seen #4, but how and what to adapt? LOL Im agree in *-sass to use *-scss as you done in #4

It's not our job to do some hacks, so best solution will be to open PR to node-sass to accept both input variants when indentedSyntax:true or even with other option.

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

3 participants